Skip to content

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

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.

Sign up for GitHub

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

Merged
seisman merged 35 commits into from
Nov 21, 2022
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1b612e8
Update legend.py
michaelgrund Aug 20, 2022
0b10141
update
michaelgrund Aug 20, 2022
172714f
update remaining functions
michaelgrund Aug 20, 2022
e22c16c
add exemplary error message
michaelgrund Aug 20, 2022
fb53e50
format
michaelgrund Aug 20, 2022
3d4934c
remove xshift in 3d scatter plot example
michaelgrund Aug 21, 2022
e5364b8
remove XY from common options
michaelgrund Aug 21, 2022
e1df24d
update decorator
michaelgrund Aug 21, 2022
3505b6b
update
michaelgrund Aug 21, 2022
b53a574
Merge branch 'main' into remove-x-y-shift
michaelgrund Aug 29, 2022
5e76789
update if clause
michaelgrund Aug 29, 2022
25e097a
format
michaelgrund Aug 29, 2022
594ec3f
Merge branch 'main' into remove-x-y-shift
michaelgrund Sep 19, 2022
444071d
Apply suggestions from code review
michaelgrund Sep 25, 2022
e195e80
Merge branch 'main' into remove-x-y-shift
michaelgrund Sep 25, 2022
94dc0ae
add updates based on timestamp/U adjustments
michaelgrund Sep 25, 2022
acb01fe
format
michaelgrund Sep 25, 2022
54ec0b0
Merge branch 'main' into remove-x-y-shift
michaelgrund Oct 2, 2022
a11ea8b
Apply suggestions from code review
michaelgrund Nov 9, 2022
bb17675
Merge branch 'main' into remove-x-y-shift
michaelgrund Nov 9, 2022
31823e3
Update decorators.py
michaelgrund Nov 9, 2022
8d62ddd
Update ternary.py
michaelgrund Nov 11, 2022
0f6f626
Update decorators.py
michaelgrund Nov 11, 2022
fef1a49
Merge branch 'main' into remove-x-y-shift
michaelgrund Nov 11, 2022
bd2b05f
Update test_config.py
michaelgrund Nov 11, 2022
a167be3
[format-command] fixes
actions-bot Nov 11, 2022
b5329fd
Merge branch 'main' into remove-x-y-shift
michaelgrund Nov 16, 2022
27d0a70
add tests
michaelgrund Nov 16, 2022
256abfd
Update test_figure.py
michaelgrund Nov 16, 2022
e80797c
[format-command] fixes
actions-bot Nov 16, 2022
4ead194
Update test_figure.py
michaelgrund Nov 16, 2022
ea0e730
Apply suggestions from code review
michaelgrund Nov 19, 2022
18bbcc8
Merge branch 'main' into remove-x-y-shift
michaelgrund Nov 19, 2022
3e76319
[format-command] fixes
actions-bot Nov 19, 2022
691d81d
Merge branch 'main' into remove-x-y-shift
michaelgrund Nov 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion examples/gallery/3d_plots/scatter3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@
zscale=1.5,
)

# Shift plot origin in x direction
fig.shift_origin(xshift=3.1)
# Add colorbar legend
fig.colorbar(xshift=3.1)
fig.colorbar()

fig.show()
17 changes: 8 additions & 9 deletions pygmt/helpers/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,6 @@
"W": """\
pen : str
Set pen attributes for lines or the outline of symbols.""",
"XY": r"""
xshift : str
[**a**\|\ **c**\|\ **f**\|\ **r**\][*xshift*].
Shift plot origin in x-direction.
yshift : str
[**a**\|\ **c**\|\ **f**\|\ **r**\][*yshift*].
Shift plot origin in y-direction. Full documentation is at
:gmt-docs:`gmt.html#xy-full`.
""",
"a": r"""
aspatial : bool or str
[*col*\ =]\ *name*\ [,...].
Expand Down Expand Up @@ -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 (

@seisman seisman Aug 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@michaelgrund michaelgrund Aug 22, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
)

@seisman seisman Aug 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

short_param in kwargs and short_param in ["X", "Y"]
):
raise GMTInvalidInput(
f"Parameters ({short_param}) and "
f"({long_alias}) are not supported anymore."
f" Please use shift_origin() instead!"
)

@weiji14 weiji14 Aug 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seisman seisman Aug 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@michaelgrund michaelgrund Aug 29, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@weiji14 weiji14 Aug 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@seisman seisman Aug 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seisman seisman Sep 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@michaelgrund michaelgrund Oct 2, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied your implemetations for this PR @seisman.

if long_alias in kwargs:
kwargs[short_param] = kwargs.pop(long_alias)
elif short_param in kwargs:
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/basemap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@
Tm="compass",
U="timestamp",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
f="coltypes",
p="perspective",
Expand Down Expand Up @@ -84,7 +82,6 @@ def basemap(self, **kwargs):
reference and anchor points
{U}
{V}
{XY}
{c}
{f}
{p}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/coast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
S="water",
U="timestamp",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
p="perspective",
t="transparency",
Expand Down Expand Up @@ -176,7 +174,6 @@ def coast(self, **kwargs):
(default is no fill). Append **+l**\|\ **+L** to =\ *continent* to
only list countries in that continent; repeat if more than one
continent is requested.
{XY}
{c}
{p}
{t}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/colorbar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
I="shading",
W="scale",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
p="perspective",
t="transparency",
Expand Down Expand Up @@ -100,7 +98,6 @@ def colorbar(self, **kwargs):
asymmetric intensity range from *low* to *high*. [Default is no
illumination].
{V}
{XY}
{c}
{p}
{t}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/contour.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
b="binary",
c="panel",
d="nodata",
Expand Down Expand Up @@ -106,7 +104,6 @@ def contour(self, data=None, x=None, y=None, z=None, **kwargs):
separator for the two labels instead.
{U}
{V}
{XY}
{b}
{c}
{d}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/grdcontour.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
V="verbose",
W="pen",
l="label",
X="xshift",
Y="yshift",
c="panel",
f="coltypes",
p="perspective",
Expand Down Expand Up @@ -83,7 +81,6 @@ def grdcontour(self, grid, **kwargs):
{U}
{V}
{W}
{XY}
{c}
{f}
label : str
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/grdimage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
R="region",
U="timestamp",
V="verbose",
X="xshift",
Y="yshift",
n="interpolation",
c="panel",
f="coltypes",
Expand Down Expand Up @@ -153,7 +151,6 @@ def grdimage(self, grid, **kwargs):
3).
{R}
{V}
{XY}
{c}
{f}
{n}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/grdview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
Wf="facadepen",
I="shading",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
f="coltypes",
n="interpolation",
Expand Down Expand Up @@ -114,7 +112,6 @@ def grdview(self, grid, **kwargs):
**+d** to select the default arguments
[Default is **+a**\ -45\ **+nt**\ 1\ **+m**\ 0].
{V}
{XY}
{c}
{f}
{n}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/histogram.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
Z="histtype",
b="binary",
c="panel",
Expand Down Expand Up @@ -124,7 +122,6 @@ def histogram(self, data, **kwargs):

To use weights provided as a second data column instead of pure counts,
append **+w**.
{XY}
{U}
{V}
{b}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
M="monochrome",
U="timestamp",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
p="perspective",
t="transparency",
Expand Down Expand Up @@ -57,7 +55,6 @@ def image(self, imagefile, **kwargs):
YIQ-transformation.
{U}
{V}
{XY}
{c}
{p}
{t}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/legend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
F="box",
U="timestamp",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
p="perspective",
t="transparency",
Expand Down Expand Up @@ -67,7 +65,6 @@ def legend(self, spec=None, position="JTR+jTR+o0.2c", box="+gwhite+p1p", **kwarg
1p black pen and adds a white background.
{U}
{V}
{XY}
{c}
{p}
{t}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/logo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
S="style",
U="timestamp",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
t="transparency",
)
Expand Down Expand Up @@ -55,7 +53,6 @@ def logo(self, **kwargs):
- **u** to place the URL to the GMT site
{U}
{V}
{XY}
{c}
{t}
"""
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/meca.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ def data_format_code(convention, component="full"):
B="frame",
N="no_clip",
V="verbose",
X="xshift",
Y="yshift",
c="panel",
p="perspective",
t="transparency",
Expand Down Expand Up @@ -219,7 +217,6 @@ def meca(
{R}
{B}
{V}
{XY}
{c}
{p}
{t}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/plot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
Z="zvalue",
a="aspatial",
b="binary",
Expand Down Expand Up @@ -184,7 +182,6 @@ def plot(self, data=None, x=None, y=None, size=None, direction=None, **kwargs):
{W}
{U}
{V}
{XY}
zvalue : str
*value*\|\ *file*.
Instead of specifying a symbol or polygon fill and outline color
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/plot3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@
S="style",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
Z="zvalue",
a="aspatial",
b="binary",
Expand Down Expand Up @@ -154,7 +152,6 @@ def plot3d(
{U}
{V}
{W}
{XY}
zvalue : str
*value*\|\ *file*.
Instead of specifying a symbol or polygon fill and outline color
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/rose.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
Z="scale",
b="binary",
d="nodata",
Expand Down Expand Up @@ -183,7 +181,6 @@ def rose(self, data=None, length=None, azimuth=None, **kwargs):

{U}
{V}
{XY}
{b}
{c}
{d}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/solar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
c="panel",
p="perspective",
t="transparency",
Expand Down Expand Up @@ -59,7 +57,6 @@ def solar(self, terminator="d", terminator_datetime=None, **kwargs):
is ``default,black,solid``.
Comment thread
michaelgrund marked this conversation as resolved.
Outdated
{U}
{V}
{XY}
{c}
{p}
{t}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/subplot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
SR="sharey",
T="title",
V="verbose",
X="xshift",
Y="yshift",
)
@kwargs_to_strings(Ff="sequence", Fs="sequence", M="sequence", R="sequence")
def subplot(self, nrows=1, ncols=1, **kwargs):
Expand Down Expand Up @@ -145,7 +143,6 @@ def subplot(self, nrows=1, ncols=1, **kwargs):
``frame``), the entire figure may also have an overarching *heading*
[no heading]. Font is determined by setting :gmt-term:`FONT_HEADING`.
{V}
{XY}
"""
kwargs = self._preprocess(**kwargs) # pylint: disable=protected-access

Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
a="aspatial",
c="panel",
e="find",
Expand Down Expand Up @@ -150,7 +148,6 @@ def text_(
Do NOT clip text at map boundaries [Default is will clip].
{U}
{V}
{XY}
{a}
{c}
{e}
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/velo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
Z="zvalue",
c="panel",
d="nodata",
Expand Down Expand Up @@ -218,7 +216,6 @@ def velo(self, data=None, **kwargs):
updated from the CPT (see ``cmap``). If instead modifier **+cf** is
appended then the color from the cpt file is applied to symbol fill
only [Default]. Use just **+c** to set both pen and fill color.
{XY}
zvalue : str
[**m**\|\ **e**\|\ **n**\|\ **u**\ ][**+e**].
Select the quantity that will be used with the CPT given via ``cmap``
Expand Down
3 changes: 0 additions & 3 deletions pygmt/src/wiggle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
U="timestamp",
V="verbose",
W="pen",
X="xshift",
Y="yshift",
Z="scale",
b="binary",
c="panel",
Expand Down Expand Up @@ -82,7 +80,6 @@ def wiggle(self, data=None, x=None, y=None, z=None, **kwargs):
{V}
pen : str
Specify outline pen attributes [Default is no outline].
{XY}
{b}
{c}
{d}
Expand Down