Skip to content
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

Add individual parameters for pen settings #1173

Open
willschlitzer opened this issue Apr 4, 2021 · 8 comments · May be fixed by #1239
Open

Add individual parameters for pen settings #1173

willschlitzer opened this issue Apr 4, 2021 · 8 comments · May be fixed by #1239
Labels
feature request New feature wanted

Comments

@willschlitzer
Copy link
Contributor

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.

@willschlitzer willschlitzer added the feature request New feature wanted label Apr 4, 2021
@seisman
Copy link
Member

seisman commented Apr 4, 2021

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

@willschlitzer
Copy link
Contributor Author

willschlitzer commented Apr 5, 2021

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
Copy link
Member

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

@seisman
Copy link
Member

seisman commented Apr 5, 2021

Also related to #1078 (comment).

@noorbuchi
Copy link
Contributor

noorbuchi commented Apr 23, 2021

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
Copy link
Contributor Author

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.

Pythonic GMT arguments automation moved this from To do to Done Apr 24, 2021
@weiji14 weiji14 linked a pull request Apr 24, 2021 that will close this issue
5 tasks
@weiji14
Copy link
Member

weiji14 commented Apr 24, 2021

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 #1082 (comment), 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 😉 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).

@weiji14 weiji14 reopened this Apr 24, 2021
Pythonic GMT arguments automation moved this from Done to In Progress Apr 24, 2021
@willschlitzer
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature wanted
Projects
Status: In Progress
Pythonic GMT arguments
  
In Progress
Development

Successfully merging a pull request may close this issue.

5 participants