search

GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.
https://www.pygmt.org
BSD 3-Clause "New" or "Revised" License
747 stars 216 forks source link

plot and plot3d: Allow passing an array as symbol #1076

Open core-man opened 3 years ago

core-man commented 3 years ago

Description of the desired feature

We can now use a 1d array for sizes and color parameters, and intensity can also accept a 1d array after #1065. Therefore, we may also support a 1d array as symbol or symbols (originally posted in https://github.com/GenericMappingTools/pygmt/pull/1065#issuecomment-801212175).

The format of the input file of GMT plot (including both -S and -I):

x y [ z ] [ size ] [ symbol-parameters ] [ intensity ] [ symbol ]

The format of the input file of GMT plot3d (including both -S and -I):

x y z [ w ] [ size ] [ symbol-parameters ] [ intensity ] [ symbol ]

Therefore, we'd also like to support an array as symbol when we use x/y in plot and x/y/z in plot3d.

See an example with x y z size intensity symbol for the GMT plot method.

gmt begin symbols png
gmt makecpt -Chot -T0/3/1
gmt plot -R0/5/0/1 -JX5c/1c -BWSne -Bxa1 -Bya0.5 -S -C -I -W1p,black << EOF
1  0.5   0   0.3  1  c
2  0.5   1   0.8 -1  t
3  0.5   2   0.5  0  i
EOF
gmt end show

symbols

Are you willing to help implement and maintain this feature? Yes, but volunteers are welcome.

weiji14 commented 3 years ago

A bit of a side issue, but do you think we can make a generic _plot function that shares some common code between plot and plot3d, as is being done with blockmean and blockmode at #1091?

core-man commented 3 years ago

A bit of a side issue, but do you think we can make a generic _plot function that shares some common code between plot and plot3d, as is being done with blockmean and blockmode at #1091?

I quickly checked the current source codes of those two methods in PyGMT, which are the same except that plot3d has an additional z parameter. So I think it's a good suggestion to make a generic _plot function.

seisman commented 3 years ago

There are still many unimplemented and complicated features/options in plot and plot3d. I would prefer NOT to make the generic _plot function until we have more thoughts on the possible features and changes.

core-man commented 3 years ago

There are still many unimplemented and complicated features/options in plot and plot3d. I would prefer NOT to make the generic _plot function until we have more thoughts on the possible features and changes.

That's true. Actually, I think I found two bugs when I try to work on this issue for the plot method. Will double-check before submitting them.

core-man commented 3 years ago

For this issue, shall we create a new parameter called symbol or symbols (which one is better?) to accept 1d-array symbols or just use the style parameter instead? I prefer to have a new parameter, just like the sizes parameter (I dont' understand why we add a s at the end) of the plot/plot3d method. How do you think?

seisman commented 3 years ago

like the sizes parameter (I dont' understand why we add a s at the end) of the plot/plot3d method

plot is one of the oldest wrappers in PyGMT, and the parameters are not carefully chosen. We had some discussions in the GMT Community meeting, and I think we agree that we will change sizes to size.

shall we create a new parameter called symbol or symbols (which one is better?) to accept 1d-array symbols or just use the style parameter instead?

I'm not sure if we should reuse style or not. If you'd like to work on this feature, I think you can use symbol first, then we can decide which one is better based on your changes.

core-man commented 3 years ago

plot is one of the oldest wrappers in PyGMT, and the parameters are not carefully chosen. We had some discussions in the GMT Community meeting, and I think we agree that we will change sizes to size.

Sound great.

shall we create a new parameter called symbol or symbols (which one is better?) to accept 1d-array symbols or just use the style parameter instead?

I'm not sure if we should reuse style or not. If you'd like to work on this feature, I think you can use symbol first, then we can decide which one is better based on your changes.

Great. I will try~

seisman commented 3 years ago

@core-man Thanks for your quick PR in #1117. Here are the possible ways to set symbols and sizes after your PR. I feel some of them are not intuitive to me.

  1. constant symbol and size: style="c0.5c"

  2. constant symbol and different sizes: style="cc" + sizes=sizes

Although we use style="cc" in our documentation, I still don't think it's not easy to understand.

  1. different symbols and constant size: style="0.5c" + symbol=symbol

I believe most users would use symbol=symbol + sizes="0.5c".

  1. different symbols and sizes: style=True + sizes=sizes + symbol=symbol

The style=True should NOT be required.

core-man commented 3 years ago
  1. constant symbol and different sizes: style="cc" + sizes=sizes

Although we use style="cc" in our documentation, I still don't think it's not easy to understand.

We can use style="c" for this case because the default unit is cetimeter. The unit can also be set by the PROJ_LENGTH_UNIT using the config method.

  1. different symbols and constant size: style="0.5c" + symbol=symbol

I believe most users would use symbol=symbol + sizes="0.5c".

That's true, but it seems that sizes cannot work for a single string or a 1d-array string now. We may refractor it. See the last comment below.

  1. different symbols and sizes: style=True + sizes=sizes + symbol=symbol

The style=True should NOT be required.

Good suggestion. Will include in #1117.

@seisman based on your comments, I think you are requesting that we should use symbol and sizes instead of style, which I also prefer because it is more intuitive. I will try to work on it.

core-man commented 3 years ago
  1. different symbols and constant size: style="0.5c" + symbol=symbol

I believe most users would use symbol=symbol + sizes="0.5c".

Shall we support a string 1d-array for the sizes parameter? Now it only accepts int and float (e.g., 1, 0.5) and does not accept string (e.g., 0.5c), although we can set the unit using the config method.

  1. different symbols and sizes: style=True + sizes=sizes + symbol=symbol

The style=True should NOT be required.

Fixed in 1c48b39. See https://github.com/GenericMappingTools/pygmt/pull/1117#issuecomment-818518810. We only need to use symbol=symbol, sizes=sizes.

weiji14 commented 3 years ago

Just following up on this. I think having 3 parameters (style/symbol/size) will get too confusing. If 'style' is not a good alias name for -S, perhaps we should deprecate it using @deprecate_parameter and rename 'style' to 'symbol' as hinted by @core-man at https://github.com/GenericMappingTools/pygmt/pull/1117#issuecomment-819267124? I.e. keep only 2 parameters (symbol and size).

core-man commented 3 years ago

Just following up on this. I think having 3 parameters (style/symbol/size) will get too confusing. If 'style' is not a good alias name for -S, perhaps we should deprecate it using @deprecate_parameter and rename 'style' to 'symbol' as hinted by @core-man at #1117 (comment)? I.e. keep only 2 parameters (symbol and size).

Good idea: First, deprecate style to symbol first, and then we could work on the remaining two parameters, i.e., symbol and size. Any comment @seisman?

weiji14 commented 1 year ago

@seisman, considering that meca supports longitude, latitude, depth, plot_longitude, plot_latitude, event_name as lists/arrays since the refactor in #1784 by putting everything into a spec dict and turning it into a pandas.DataFrame, could we do something similar here for plot/plot3d to handle symbols? This chunk of code in particular:

https://github.com/GenericMappingTools/pygmt/blob/6c69fb6bdf80cc667621376ea7bbd1b4a24f82ec/pygmt/src/meca.py#L319-L332

The spec dict is then converted to a pandas.DataFrame, and passed in to lib.virtualfile_from_data via the data parameter here:

https://github.com/GenericMappingTools/pygmt/blob/6c69fb6bdf80cc667621376ea7bbd1b4a24f82ec/pygmt/src/meca.py#L369

Maybe we could create a reusable function out of this somehow?

seisman commented 1 year ago

Maybe we could create a reusable function out of this somehow?

Sounds a promising idea.