Skip to content

Remove the xshift (X) and yshift (Y) aliases from all plotting modules? #924

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

Closed
seisman opened this issue Feb 17, 2021 · 6 comments
Closed
Labels
enhancement Improving an existing feature
Milestone

Comments

@seisman
Copy link
Member

seisman commented Feb 17, 2021

In the Zen of Python:

There should be one-- and preferably only one --obvious way to do it.

Now we have two different methods to shift origins:

  1. passing arguments like xshift="2c", yshift="1c" to any plotting modules
  2. explicitly call Figure.shift_origin(xshift="2c", yshift="1c")

GMT users are familiar with method 1, but IMHO method 2 is more pythonic. As I can see, these two methods are equivalent.

Should we remove xshift and yshift from all plotting modules, so that we can better follow the zen of python, and also make the docstrings shorter.

If yes, we need to do more things:

  1. Remove xshift and yshift alias from docstrings of all plotting functions
  2. Raise a warning/error if xshift, yshift, X or Y is passed.
  3. Improve the docstrings of Figure.shift_origin().
@seisman seisman added the question Further information is requested label Feb 17, 2021
@seisman
Copy link
Member Author

seisman commented Feb 18, 2021

Similar to this issue, I also don't like the timestamp (-U) alias. I'm in favor of removing timestamp too, and add a new method Figure.timestamp().

We can discuss xshift and yshift first. If we all agree, I'll open another issue to better track and discuss the Figure.timestamp() method.

@seisman
Copy link
Member Author

seisman commented Feb 20, 2021

Ping @GenericMappingTools/python and @GenericMappingTools/python-maintainers for comments and thoughts.

@weiji14
Copy link
Member

weiji14 commented Feb 20, 2021

For context, the xshift/yshift aliases were added in #624. While I agree with that line in the Zen of Python "There should be one-- and preferably only one --obvious way to do it.", what comes higher up before that line is "Readability counts.", and that is what the xshift/yshift aliases (and other PyGMT aliases) was originally intended for.

  1. Improve the docstrings of Figure.shift_origin().

Agree that this should be done, regardless of whether we decide to deprecate xshift (X) and yshift (Y).

  1. Remove xshift and yshift alias from docstrings of all plotting functions
  2. Raise a warning/error if xshift, yshift, X or Y is passed.

To be honest, this would be a good opportunity to practice Deprecation Warning cycles in PyGMT. But I'm not keen on handling this work, so my vote would be to keep the existing behaviour (xshift/yshift still works), but I'll respect people's opinions if deprecating xshift/yshift is a more popular opinion.

@maxrjones
Copy link
Member

Now that we have a deprecation policy, I support deprecating xshift and yshift in favor of Figure.shift_origin().

@leouieda
Copy link
Member

That's a +1 from me as well. A good first step is to not use them in docs even if they are still available. This cuts down on the docstring repetition as well.

@seisman
Copy link
Member Author

seisman commented Mar 7, 2023

If yes, we need to do more things:

  1. Remove xshift and yshift alias from docstrings of all plotting functions
  2. Raise a warning/error if xshift, yshift, X or Y is passed.

This is already done in #2071.

  1. Improve the docstrings of Figure.shift_origin().

This is not done yet but will be tracked in issue #2401.

Similar to this issue, I also don't like the timestamp (-U) alias. I'm in favor of removing timestamp too, and add a new method Figure.timestamp().

We can discuss xshift and yshift first. If we all agree, I'll open another issue to better track and discuss the Figure.timestamp() method.

Done in #2208 and #2135.

Now we have two different methods to shift origins:

  1. passing arguments like xshift="2c", yshift="1c" to any plotting modules
  2. explicitly call Figure.shift_origin(xshift="2c", yshift="1c")

GMT users are familiar with method 1, but IMHO method 2 is more pythonic. As I can see, these two methods are equivalent.

This is not exactly true. See #2401 for explanations and solutions.

I'm closing the issue now and we can have more discussions in #2401.

@seisman seisman closed this as completed Mar 7, 2023
@seisman seisman added this to the 0.9.0 milestone Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants