GenericMappingTools / pygmt

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

Add individual parameters for pen settings #1173

Open willschlitzer opened 3 years ago

willschlitzer commented 3 years ago

The current setup for the pen parameter involves passing the literal GMT string. I think that this is confusing and non-intuitive for individuals who haven't used GMT. I think that the PyGMT wrappers for GMT modules that take a pen parameter should have parameters such pen_color, pen_width, pen_color, which will then be inputs to a function that returns a GMT-formatted string that is passed to the GMT API.

Are you willing to help implement and maintain this feature? Yes! I have a pretty good idea of how this would work, but want to gauge community interest before creating a pull request.

seisman commented 3 years ago

@willschlitzer Could you please give an example showing what the new syntax looks like?

willschlitzer commented 3 years ago

Sure! Here is my rough idea using fig.plot.

@kwargs_to_strings(R="sequence", c="sequence_comma", i="sequence_comma", p="sequence")
def plot(
    self,
    x=None,
    y=None,
    data=None,
    sizes=None,
    direction=None,
    pen_width="",
    pen_color="",
    pen_style="",
    **kwargs,
):
    if pen_width or pen_color or pen_style:
        kwargs["W"] = pen_string_generator(
            pen_width=pen_width, pen_style=pen_style, pen_color=pen_color
        )

Here is the basic overview of the function:

def pen_string_generator(pen_width="", pen_color="", pen_style="") -> str:
    pen_width = str(pen_width)
    return f"{pen_width}p,{pen_color},{pen_style}"

The pen_string_generator function would be further expanded if only certain arguments are passed (such as only pen_width and pen_style but no pen_color), and adding in different line attributes (+o, +v, etc.) that would be appended onto the -W string. That second part is what I'm most unsure about, as that seems like a lot of pen-specific parameters (most of them rarely used) that would need to be in both plot (and other plotting modules that have the pen parameter) and pen_string_generator.

As I understand it, this could/would eventually become a breaking change for using the pen parameter. My thought process is that PyGMT should move towards having more Python-like variables instead of making users learn GMT syntax with the only difference being that they replace the GMT parameter with a Python function parameter (much like #356 advocating moving away from the GMT string passed after -J), and that this is one of the easier first steps to take.

maxrjones commented 3 years ago

Could the implementation be designed to work for both parameters such as pen and parameters that include **+f**font amongst the supported modifiers (for example, the velo scaling parameter)? This would probably require sorting out #1082.

seisman commented 3 years ago

Also related to https://github.com/GenericMappingTools/pygmt/issues/1078#issuecomment-803246677.

noorbuchi commented 3 years ago

Hello @seisman @willschlitzer, my team and I found this issue interesting and we're wondering if we can work on it or if it's already claimed or discussion is still in progress. We would also appreciate any suggestions you have about another issue we can work on if this one is not ready yet

willschlitzer commented 3 years ago

Hello @seisman @willschlitzer, my team and I found this issue interesting and we're wondering if we can work on it or if it's already claimed or discussion is still in progress. We would also appreciate any suggestions you have about another issue we can work on if this one is not ready yet

Hello @noorbuchi! I think this issue is part of the larger debate on how PyGMT should move towards more Pythonic arguments rather than using GMT syntax. I'm going to close this issue to avoid duplication of efforts on different issues, but please check out the references issues above that discuss this.

weiji14 commented 3 years ago

I started an RFC (request for comments) PR at #1239 to tackle this issue, so I'm reopening it to continue the discussion. As I mentioned in https://github.com/GenericMappingTools/pygmt/issues/1082#issuecomment-814040028, it will be great to have separate issues for the different parameters we want to have convenience classes for (e.g. Position (-D), Pen (-W), AreaFill (-G) etc).

@noorbuchi (and team), you are welcome to leave review comments on #1239. It will be good for you to experience the code review side of open source too :wink: If you want, we can also open up a separate issue (once #1239 is completed) for making a new convenience class like AreaFill (-G) (refer to https://docs.generic-mapping-tools.org/latest/cookbook/features.html#specifying-area-fill-attributes).

willschlitzer commented 2 years ago

I think having the user need to use a separate class (as in #1239) to set the pen information outside of the plotting function overcomplicates the process. I think the best bet for overall usability is to have a few parameters (something like pen_width, pen_color, and pen_style) that could call the Pen class from inside the function to build the string to pass to the GMT API. There could also be something like an string_append (my quick example name) parameter to add things like +o and +v, but I also think that those are less used than the main pen attributes, and supporting them should be prioritized lower.