Skip to content

Clarify that the "transparency" parameter in plot/plot3d/text can be 1d array #1265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 20, 2021

Conversation

core-man
Copy link
Member

@core-man core-man commented May 9, 2021

Description of proposed changes

Fixes #1098

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@core-man core-man marked this pull request as draft May 9, 2021 11:41
@core-man
Copy link
Member Author

core-man commented May 9, 2021

I am going to add 1d array transparency test, so the following functions should be renamed to a general one. How about test_plot_fail_1d_array_with_data and test_plot3d_fail_1d_array_with_data (done in 8eb1369).

def test_plot_fail_color_size_intensity(data):
"""
Should raise an exception if array color, size and intensity are used with
matrix.
"""

def test_plot3d_fail_color_size_intensity(data, region):
"""
Should raise an exception if array color, size and intensity are used with
matrix.

@core-man
Copy link
Member Author

I am wondering if we should add 1d array in the following COMMON_OPTIONS. Currently, some modules still do not support 1d array transparency, e.g, wiggle, since we may update the alias later.

"t": """\
transparency : int or float

@core-man core-man marked this pull request as ready for review May 19, 2021 07:23
@seisman
Copy link
Member

seisman commented May 19, 2021

I am wondering if we should add 1d array in the following COMMON_OPTIONS. Currently, some modules still do not support 1d array transparency, e.g, wiggle, since we may update the alias later.

"t": """\
transparency : int or float

In GMT CLI, only a few modules (e.g., plot, plot3d, text) support varying transparency.

@seisman seisman added this to the 0.4.0 milestone May 19, 2021
@seisman seisman added the documentation Improvements or additions to documentation label May 19, 2021
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label May 19, 2021
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one tiny suggestion :)

In GMT CLI, only a few modules (e.g., plot, plot3d, text) support varying transparency.

Seems like we're missing the varying transparency feature for text, maybe open an issue for this, and we can get someone to work on it (after #1121).

@core-man
Copy link
Member Author

core-man commented May 20, 2021

In GMT CLI, only a few modules (e.g., plot, plot3d, text) support varying transparency.

Seems like we're missing the varying transparency feature for text, maybe open an issue for this, and we can get someone to work on it (after #1121).

We have varying transparency feature for text, no?

pygmt/pygmt/src/text.py

Lines 200 to 204 in 5125224

# If an array of transparency is given, GMT will read it from
# the last numerical column per data record.
if "t" in kwargs and is_nonstr_iter(kwargs["t"]):
extra_arrays.append(kwargs["t"])
kwargs["t"] = ""

def test_text_varying_transparency():
"""
Add texts with varying transparency.
"""

Or you mean improving the documentation for the transparency option in text?

@weiji14
Copy link
Member

weiji14 commented May 20, 2021

Oh yes, forgot that varying transparency for text was added in #716. Could you please add it to the documentation for text then? And change the title of this PR too after.

@core-man
Copy link
Member Author

Oh yes, forgot that varying transparency for text was added in #716. Could you please add it to the documentation for text then? And change the title of this PR too after.

Done in 505ec41 and c3d0223. But it seems that the transparency parameter in text should be polished a little, e.g., raise a GMTInvalidInput if 1d array transparency is not used with x/y.

@core-man core-man changed the title Clarify 1d array transparency in plot/plot3d Clarify that the "transparency" parameter in plot/plot3d/text can be 1d array May 20, 2021
@weiji14
Copy link
Member

weiji14 commented May 20, 2021

Oh yes, forgot that varying transparency for text was added in #716. Could you please add it to the documentation for text then? And change the title of this PR too after.

Done in 505ec41 and c3d0223. But it seems that the transparency parameter in text should be polished a little, e.g., raise a GMTInvalidInput if 1d array transparency is not used with x/y.

Yes, but that can be done in a separate Pull Request, let's keep this one focused on just the docstring (and unit test). I think @seisman is doing a bit of work on refactoring the text function at #1121 so best not to cause conflicts.

P.S. The ubuntu-latest - Python 3.9 / NumPy 1.20 tests are failing due to an unrelated reason at #1278. I'll see if I can come up with a quick fix, if not, it should be fine to merge (as an admin) since the other tests are passing.

@seisman seisman merged commit c411377 into master May 20, 2021
@seisman seisman deleted the plot-transparency branch May 20, 2021 19:32
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label May 20, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The transparency parameter of plot/plot3d methods can be 1d array
3 participants