Deprecate xshift (X) and yshift (Y) aliases from all plotting modules (remove in v0.12.0) #2071

michaelgrund · 2022-08-20T16:21:25Z

Description of proposed changes

This PR addresses #924 and removes the xshift (X) and yshift (Y) aliases from all plotting modules and gallery examples.

Modules:

Gallery examples:

scatter3d.py

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

michaelgrund · 2022-08-20T17:04:00Z

Should we add the warning/error if xshift, yshift, X or Y are passed directly into each function or create a correspondig decorator @GenericMappingTools/pygmt-maintainers? Exemplary I added one in basemap.py.

seisman · 2022-08-21T01:35:51Z

Should we add the warning/error if xshift, yshift, X or Y are passed directly into each function or create a correspondig decorator @GenericMappingTools/pygmt-maintainers? Exemplary I added one in basemap.py.

I feel it's possible to add the warning in the use_alias decorator, instead of creating a new decorator.

michaelgrund · 2022-08-21T10:46:14Z

Should we add the warning/error if xshift, yshift, X or Y are passed directly into each function or create a correspondig decorator @GenericMappingTools/pygmt-maintainers? Exemplary I added one in basemap.py.

I feel it's possible to add the warning in the use_alias decorator, instead of creating a new decorator.

I updated the use_alias decorator. Is this the direction you had in mind @seisman?

seisman · 2022-08-21T14:13:03Z

Should we add the warning/error if xshift, yshift, X or Y are passed directly into each function or create a correspondig decorator @GenericMappingTools/pygmt-maintainers? Exemplary I added one in basemap.py.

I feel it's possible to add the warning in the use_alias decorator, instead of creating a new decorator.

I updated the use_alias decorator. Is this the direction you had in mind @seisman?

Yes. Does it work as expected?

seisman · 2022-08-22T05:58:55Z

pygmt/helpers/decorators.py

@@ -574,6 +565,14 @@ def new_module(*args, **kwargs):
                        f"Parameters in short-form ({short_param}) and "
                        f"long-form ({long_alias}) can't coexist."
                    )
+                if (long_alias in kwargs and long_alias in ["xshift", "yshift"]) or (


I think it doesn't works, because after removing X="xshift" and Y="yshift" from the use_alias decorator, the variable aliases at line 571 no longer contain the paramters xshift or yshift.

Instead, after line 583, you can check if any of xshift, yshift, X and Y exists in kwargs. If yes, then raise an warning. For backward compatibility, you also need to do the "xshift"->X and "yshift"->"Y" conversions if "xshift" or "yshift" is used.

So, you mean to leave X="xshift" and Y="yshift" as is in each function and add the others at this point, too @seisman ?

@use_alias( X="xshift", Y="yshift", xshift="X", yshift="Y", )

So, you mean to leave X="xshift" and Y="yshift" as is in each function and add the others at this point, too @seisman ?

@use_alias( X="xshift", Y="yshift", xshift="X", yshift="Y", )

No. Changes to other files (e.g., basemap.py) make sense to me. The only thing that is missing is how to correctly check if users use xshift, yshift, X or Y. The checking should be done in the pygmt/helpers/decorators.py file.

weiji14 · 2022-08-22T13:22:16Z

pygmt/helpers/decorators.py

+                    raise GMTInvalidInput(
+                        f"Parameters ({short_param}) and "
+                        f"({long_alias}) are not supported anymore."
+                        f" Please use shift_origin() instead!"
+                    )


Using an error here will immediately break people's code if they're using xshift or yshift. Better to go through a deprecation cycle that raises a FutureWarning instead.

One idea is to use the @deprecate_parameter function and do something like @deprecate_parameter(oldname="xshift", newname=None, deprecate_version="v0.8.0", remove_version="0.10.0"). This will require some modifications to the decorator to handle newname=None though.

One idea is to use the @deprecate_parameter function and do something like @deprecate_parameter(oldname="xshift", newname=None, deprecate_version="v0.8.0", remove_version="0.10.0"). This will require some modifications to the decorator to handle newname=None though.

We don't have to use @deprecate_parameter for this, which requires adding @deprecate_parameter decorators in many Python files.

I modified the passage @seisman. I understand the concerns @weiji14 has regarding raising an error here. However, we did agree to totally drop this option (not only deprecate it), didn't we?

Looking at #924 (comment), I think the intention was to remove xshift/yshift from the docstring, not completely from the code. I.e. the aliases should still work (with a FutureWarning) for at least 2 more minor versions. But maybe I'm missing some context here, was there a discussion to drop xshift/yshift in a backward incompatible way?

was there a discussion to drop xshift/yshift in a backward incompatible way?

No, we definitely need backward compatibility.

The final goal is, for the next 2 or more minor versions, users can still use parameters xshift, yshift, X, Y, and they will see a FutureWarning like:

Parameters xshift, yshift, X and Y are deprecated and will be no longer supported in v0.x.x. Please user Figure.shift_origin() instead.

One idea is to use the @deprecate_parameter function and do something like @deprecate_parameter(oldname="xshift", newname=None, deprecate_version="v0.8.0", remove_version="0.10.0"). This will require some modifications to the decorator to handle newname=None though.

I believe this will work but it's not ideal. It's easy to modify the deprecate_parameter decorator to support newname=None, but the warning message would be something like:

The 'xxxx' parameter has been deprecated since vx.x.x and will be removed in vx.x.x.

The error message can't recommend using Figure.shift_origin() unless we deal with xshift and yshfit in a special way.

It's also more confusing when users use the short parameters X or Y. When they use X, they are recommended to use xshift. After they modify X to xshift, then they see xshfit is deprecated.

I've opened PR #2135, which shows how we can deprecate timestamp (U) in a backward-compatible way and recommend Figure.timestamp() (not implemented yet) instead. The same idea can be applied to xshift and yshift in this PR.

I applied your implemetations for this PR @seisman.

michaelgrund · 2022-11-11T13:03:47Z

/format

michaelgrund · 2022-11-16T19:05:03Z

/format

michaelgrund · 2022-11-16T20:33:26Z

Some changes need to be made:

Need to remove xshift and yshift from https://github.com/GenericMappingTools/pygmt/blob/main/pygmt/src/ternary.py

xshift and yshift are still used in https://github.com/GenericMappingTools/pygmt/blob/main/pygmt/tests/test_config.py.

Placeholder xyshift is no longer used and can be removed from COMMON_DOCSTRING:

pygmt/pygmt/helpers/decorators.py

Line 100 in 6964fe1

"xyshift": r"""

Need to add a test to make sure that a warning is raised if users use xshift/X/yshift/Y

Added 4 tests covering each parameter. Do we need all 4 or is one sufficient @seisman ?

seisman · 2022-11-16T23:37:31Z

Some changes need to be made:

Need to remove xshift and yshift from main/pygmt/src/ternary.py

xshift and yshift are still used in main/pygmt/tests/test_config.py.

Placeholder xyshift is no longer used and can be removed from COMMON_DOCSTRING:

pygmt/pygmt/helpers/decorators.py

Line 100 in 6964fe1

"xyshift": r"""

Need to add a test to make sure that a warning is raised if users use xshift/X/yshift/Y

Added 4 tests covering each parameter. Do we need all 4 or is one sufficient @seisman ?

I think we can merge the four tests into one larger test.

willschlitzer · 2022-11-17T13:34:02Z

pygmt/tests/test_figure.py

+def test_figure_depr_x():
+    """
+    Check if deprecation of X parameter works correctly if used.
+    """
+    fig = Figure()
+    fig.basemap(region=[0, 1, 0, 1], projection="X1c/1c", frame=True)
+    with pytest.warns(expected_warning=SyntaxWarning) as record:
+        fig.plot(x=1, y=1, style="c3c", X="3c")
+        assert len(record) == 1  # check that only one warning was raised


Does the testing of X and Y needs to be included if xshift and yshift are already included, since we trust that the aliases translate to the appropriate arguments?

Does the testing of X and Y needs to be included if xshift and yshift are already included, since we trust that the aliases translate to the appropriate arguments?

I think we still need to test X and Y, because we have removed the "X=>xshift" and "Y=>yshift" aliases in this PR.

pygmt/tests/test_figure.py

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

seisman · 2022-11-19T14:54:01Z

/format

seisman · 2022-11-20T04:41:17Z

Ping @GenericMappingTools/pygmt-maintainers for final reviews.

willschlitzer

Nice work @michaelgrund!

… (remove in v0.12.0) (GenericMappingTools#2071) Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

Update legend.py

1b612e8

michaelgrund added documentation Improvements or additions to documentation deprecation Deprecating a feature labels Aug 20, 2022

michaelgrund added this to the 0.8.0 milestone Aug 20, 2022

michaelgrund changed the title ~~Remove xshift (X) and yshift (Y) aliases from all plotting modules~~ WIP: Remove xshift (X) and yshift (Y) aliases from all plotting modules Aug 20, 2022

michaelgrund marked this pull request as draft August 20, 2022 16:21

michaelgrund added 3 commits August 20, 2022 18:33

update

0b10141

update remaining functions

172714f

add exemplary error message

e22c16c

michaelgrund requested review from willschlitzer, seisman and weiji14 and removed request for willschlitzer and seisman August 20, 2022 17:04

format

fb53e50

michaelgrund added 3 commits August 21, 2022 11:23

remove xshift in 3d scatter plot example

3d4934c

remove XY from common options

e5364b8

update decorator

e1df24d

update

3505b6b

seisman reviewed Aug 22, 2022

View reviewed changes

weiji14 reviewed Aug 22, 2022

View reviewed changes

michaelgrund and others added 3 commits August 29, 2022 18:52

Merge branch 'main' into remove-x-y-shift

b53a574

update if clause

5e76789

format

25e097a

michaelgrund added 2 commits November 11, 2022 13:54

Merge branch 'main' into remove-x-y-shift

fef1a49

Update test_config.py

bd2b05f

actions-bot and others added 4 commits November 11, 2022 13:04

[format-command] fixes

a167be3

Merge branch 'main' into remove-x-y-shift

b5329fd

add tests

27d0a70

Update test_figure.py

256abfd

actions-bot and others added 2 commits November 16, 2022 19:05

[format-command] fixes

e80797c

Update test_figure.py

4ead194

willschlitzer reviewed Nov 17, 2022

View reviewed changes

seisman reviewed Nov 19, 2022

View reviewed changes

pygmt/tests/test_figure.py Outdated Show resolved Hide resolved

michaelgrund and others added 2 commits November 19, 2022 10:57

Apply suggestions from code review

ea0e730

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

Merge branch 'main' into remove-x-y-shift

18bbcc8

[format-command] fixes

3e76319

seisman approved these changes Nov 19, 2022

View reviewed changes

seisman added final review call This PR requires final review and approval from a second reviewer and removed documentation Improvements or additions to documentation labels Nov 19, 2022

Merge branch 'main' into remove-x-y-shift

691d81d

willschlitzer approved these changes Nov 21, 2022

View reviewed changes

seisman merged commit 91bfe28 into main Nov 21, 2022

seisman deleted the remove-x-y-shift branch November 21, 2022 15:19

seisman removed the final review call This PR requires final review and approval from a second reviewer label Nov 21, 2022

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022

Deprecate xshift (X) and yshift (Y) aliases from all plotting modules…

b979192

… (remove in v0.12.0) (GenericMappingTools#2071) Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

seisman mentioned this pull request Mar 7, 2023

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

Closed

yvonnefroehlich mentioned this pull request Feb 6, 2024

Figure: Remove deprecated "xshift" ("X") and "yshift" ("Y") parameters, use "Figure.shift_origin" instead (deprecated since v0.8.0) #3044

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate xshift (X) and yshift (Y) aliases from all plotting modules (remove in v0.12.0) #2071

Deprecate xshift (X) and yshift (Y) aliases from all plotting modules (remove in v0.12.0) #2071

michaelgrund commented Aug 20, 2022 •

edited

Loading

michaelgrund commented Aug 20, 2022

seisman commented Aug 21, 2022

michaelgrund commented Aug 21, 2022

seisman commented Aug 21, 2022

seisman Aug 22, 2022

michaelgrund Aug 22, 2022

seisman Aug 22, 2022

weiji14 Aug 22, 2022

seisman Aug 22, 2022

michaelgrund Aug 29, 2022

weiji14 Aug 29, 2022

seisman Aug 30, 2022

seisman Sep 24, 2022 •

edited

Loading

michaelgrund Oct 2, 2022

michaelgrund commented Nov 11, 2022

michaelgrund commented Nov 16, 2022

michaelgrund commented Nov 16, 2022

seisman commented Nov 16, 2022

willschlitzer Nov 17, 2022

seisman Nov 18, 2022

seisman commented Nov 19, 2022

seisman commented Nov 20, 2022

willschlitzer left a comment

Deprecate xshift (X) and yshift (Y) aliases from all plotting modules (remove in v0.12.0) #2071

Deprecate xshift (X) and yshift (Y) aliases from all plotting modules (remove in v0.12.0) #2071

Conversation

michaelgrund commented Aug 20, 2022 • edited Loading

michaelgrund commented Aug 20, 2022

seisman commented Aug 21, 2022

michaelgrund commented Aug 21, 2022

seisman commented Aug 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seisman Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgrund commented Nov 11, 2022

michaelgrund commented Nov 16, 2022

michaelgrund commented Nov 16, 2022

seisman commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seisman commented Nov 19, 2022

seisman commented Nov 20, 2022

willschlitzer left a comment

Choose a reason for hiding this comment

michaelgrund commented Aug 20, 2022 •

edited

Loading

seisman Sep 24, 2022 •

edited

Loading