-
Notifications
You must be signed in to change notification settings - Fork 222
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
Figure.plot: Add the "symbol" parameter to support plotting data points with varying symbols #1117
Figure.plot: Add the "symbol" parameter to support plotting data points with varying symbols #1117
Conversation
Therefore, now we can plot different symbols and sizes like this: |
pygmt/src/plot.py
Outdated
@@ -206,15 +210,18 @@ def plot(self, x=None, y=None, data=None, sizes=None, direction=None, **kwargs): | |||
kind = data_kind(data, x, y) | |||
|
|||
extra_arrays = [] | |||
if "S" in kwargs and kwargs["S"][0] in "vV" and direction is not None: | |||
extra_arrays.extend(direction) | |||
if "S" in kwargs and isinstance(kwargs["S"], str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added isinstance(kwargs["S"], str)
in case that the style
parameter is True, False, None, int, or float.
@core-man I think the co-existence of |
Actually, I'd like to fully avoid using the We could remove the |
I believe we need more discussions about what we would like to see about the |
pygmt/tests/test_plot.py
Outdated
projection="X4c", | ||
color="blue", | ||
sizes=np.full(len(data[:, 0]), 0.5), | ||
symbol=np.full(len(data[:, 0]), "c"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, this test is checking that plotting all symbols as circles (c) work. A better test would be to test a mix of symbols such as circles (c), triangles (t), squares (s), etc (like your example in #1076 (comment)).
The CI failures are caused by the codecov step. It's likely that codecov can't get the OIDC token for forks. Edit: Likely related to upstream issue at codecov/codecov-action#1594. |
@@ -87,6 +89,8 @@ def plot(self, data=None, x=None, y=None, size=None, direction=None, **kwargs): | |||
size : 1-D array | |||
The size of the data points in units specified using ``style``. | |||
Only valid if using ``x``/``y``. | |||
symbol : 1-D array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbol can also be a single string right? E.g. symbol="t"
for triangle. I think we need to add another unit test for the "constant-symbol and different sizes" case as mentioned at #1117 (comment)
symbol : 1-D array | |
symbol : str or 1-D array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, similar to the size parameter, symbol must be a 1-D array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ok, I just tried with symbol="t"
and it plotted a straight line:
import pygmt
fig = pygmt.Figure()
fig.plot(
x=[1, 2, 3, 4],
y=[1, 1, 1, 1],
region=[0, 5, 0, 2],
projection="X2c",
fill="blue",
size=[0.1, 0.2, 0.3, 0.4],
symbol="t",
frame="af",
)
fig.show()
So yes, it does have to be a 1-D array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Maybe writing "list of str" is clearer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1-D array is used here to match the types of other parameters. We probably can change it to Sequence[str] when we add typing hints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers @seisman for getting this over the line, I've tested this locally and it looks good. The changes look simple but there were a lot of refactorings in the PyGMT codebase to get this to work! Also thanks again @core-man for starting this effort 3 years ago, we'll finally be able to release this in PyGMT v0.14.0 🎉
P.S. We should also port these changes to plot3d
later.
@@ -87,6 +89,8 @@ def plot(self, data=None, x=None, y=None, size=None, direction=None, **kwargs): | |||
size : 1-D array | |||
The size of the data points in units specified using ``style``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. But if size
is used with symbol
the unit for size
is given by the GMT / PyGMT defaults, correct?
Using style
for specifing the unit in this case would be super confusing, e.g.:
...
size=[0.5, 0.8, 0.9],
symbol=["s", "c", "d"],
style="c", # now centimeters, not circle
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it produces three circles with different sizes.
I want to emphasize that the "style" parameter was introduced in very early PyGMT versions. The docstrings are mostly copied from the upstream GMT documentation without further polishes, so they may make no sense for PyGMT.
projection="X4c", | ||
fill="blue", | ||
size=[0.1, 0.2, 0.3, 0.4], | ||
symbol=["c", "t", "i", "s"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do size
and symbol
need to have to same length? Or is something like:
size=[0.1, 0.2, 0.3, 0.4], # different sizes but same symbol
symbol=["c"],
or
size=[0.4], # same size but different symbols
symbol=["c", "t", "i", "s"],
allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, they must be the same size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I figured it out: One has to use the size
or symbol
parameter together with the style
parameter 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this introduction part in the docs for Figure.plot
needs some adjustment to account for this newly added symbol
parameter and to avoid confusions:
Lines 65 to 70 in ce89ccb
If a symbol is selected and no symbol size given, then plot will | |
interpret the third column of the input data as symbol size. Symbols | |
whose size is <= 0 are skipped. If no symbols are specified then the | |
symbol code (see ``style`` below) must be present as last column in the | |
input. If ``style`` is not used, a line connecting the data points will | |
be drawn instead. To explicitly close polygons, use ``close``. Select a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely a nice enhancement!
So far, I only made some comments (will be back to work at the end of this week).
I feel there can be some confusions regarding the style
, symbol
, and size
parameters. Maybe we should also extend or add a gallery example for this new parameter?
Currently, docstrings of most PyGMT methods/functions are copied literally from the upstream GMT documentation and may make no sense to PyGMT. I think we need to polish most docstrings carefully.
Yes, definitely. |
Description of proposed changes
We can only plot different symbols via the
data
parameter in the plot method. This PR allows passing an array as symbols via the newsymbol
parameter. Meanwhile, we have to setstyle="0.2c"
, or set 1d-array sizes usingstyle=True
andsizes=sizes
. Therefore, we now have four ways to set symbols and sizes, e.g.,style="c0.5c"
style="cc"
+sizes=sizes
style="0.5c"
+symbol=symbol
style=True
+sizes=sizes
+symbol=symbol
Two tests are added:
test_plot_fail_1d_array_using_data
: RaiseGMTInvalidInput
ifsymbol
is used withdata
parameter. Actually, I renametest_plot_fail_color_size_intensity
to be this test function since we should raise such an error for 1d-arraycolor
,size
,intensity
andsymbol
parameters if they are used with thedata
parameter. (originally posted in The transparency parameter of plot/plot3d methods can be 1d array #1098 (comment))test_plot_symbols
: test 1d-arraysymbol
(all are"c"
) withstyle="0.5c"
.Related #1076
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version