-
Notifications
You must be signed in to change notification settings - Fork 235
New Alias System: Add the AliasSystem class #4000
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
Changes from 19 commits
654261c
7f43055
a42c2d8
bc0401e
fae3ab0
f4f3012
cd536bb
b0f0d14
b244f74
ddcc7b8
7ff71d1
c1fdfa5
571d0dd
d0ebbcb
408199c
76f7c6e
657f053
324b973
a375e69
99845c2
5259b1e
92e0a08
5a9817d
195acfa
4cdef8b
e57954d
4ce43d1
209fda1
8d2e1be
7e3fb66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,17 +2,15 @@ | |||||||||||||||||||||
| basemap - Plot base maps and frames. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from pygmt.alias import Alias, AliasSystem | ||||||||||||||||||||||
|
||||||||||||||||||||||
| from pygmt.clib import Session | ||||||||||||||||||||||
| from pygmt.helpers import build_arg_list, fmt_docstring, kwargs_to_strings, use_alias | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @fmt_docstring | ||||||||||||||||||||||
| @use_alias( | ||||||||||||||||||||||
| R="region", | ||||||||||||||||||||||
| J="projection", | ||||||||||||||||||||||
| Jz="zscale", | ||||||||||||||||||||||
| JZ="zsize", | ||||||||||||||||||||||
| B="frame", | ||||||||||||||||||||||
| L="map_scale", | ||||||||||||||||||||||
| F="box", | ||||||||||||||||||||||
| Td="rose", | ||||||||||||||||||||||
|
|
@@ -23,8 +21,8 @@ | |||||||||||||||||||||
| p="perspective", | ||||||||||||||||||||||
| t="transparency", | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| @kwargs_to_strings(R="sequence", c="sequence_comma", p="sequence") | ||||||||||||||||||||||
| def basemap(self, **kwargs): | ||||||||||||||||||||||
| @kwargs_to_strings(c="sequence_comma", p="sequence") | ||||||||||||||||||||||
| def basemap(self, region=None, projection=None, frame=None, **kwargs): | ||||||||||||||||||||||
| r""" | ||||||||||||||||||||||
| Plot base maps and frames. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -83,5 +81,12 @@ def basemap(self, **kwargs): | |||||||||||||||||||||
| {transparency} | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| self._activate_figure() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| alias = AliasSystem( | ||||||||||||||||||||||
| R=Alias(region, name="region", separator="/", size=[4, 6]), | ||||||||||||||||||||||
| J=Alias(projection, name="projection"), | ||||||||||||||||||||||
| B=Alias(frame, name="frame"), | ||||||||||||||||||||||
| ).update(kwargs) | ||||||||||||||||||||||
|
||||||||||||||||||||||
| alias = AliasSystem( | |
| R=Alias(region, name="region", separator="/", size=[4, 6]), | |
| J=Alias(projection, name="projection"), | |
| B=Alias(frame, name="frame"), | |
| ).update(kwargs) | |
| kwdict = AliasSystem( | |
| R=Alias(region, name="region", separator="/", size=[4, 6]), | |
| J=Alias(projection, name="projection"), | |
| B=Alias(frame, name="frame"), | |
| ).update(kwargs).kwdict |
since only alias.kwdict is used below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did wonder if we could turn the AliasSystem class into a subclass of collections.UserDict or collections.OrderedDict (or maybe collections.ChainMap?), since it is essentially just holding a dictonary with a custom update method. But then you'll need to reconcile self.aliasdict with self.kwdict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did wonder if we could turn the
AliasSystemclass into a subclass ofcollections.UserDictorcollections.OrderedDict(or maybecollections.ChainMap?), since it is essentially just holding a dictonary with a customupdatemethod.
Then we will have codes like
alias = AliasSystem(A=Alias(...), B=Alias(...), ...)
build_arg_list(alias)
or rename alias to kwdict, but both may be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or rename
aliastokwdict, but both may be confusing.
how about aliasdict = AliasSystem(...)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aliasdict looks good.
I just tried UserDict and it looks good. Here is a minimal example:
from collections import UserDict
from collections.abc import Sequence
from pygmt.alias import Alias
class AliasSystem(UserDict):
def __init__(self, **kwargs):
self.aliasdict = kwargs
kwdict = {}
for option, aliases in kwargs.items():
if isinstance(aliases, Sequence):
values = [alias._value for alias in aliases if alias._value is not None]
if values:
kwdict[option] = "".join(values)
elif aliases._value is not None:
kwdict[option] = aliases._value
super().__init__(kwdict)
def update(self, kwargs):
print("Updating dict")
# Add more checks later.
for short_param, value in kwargs.items():
self[short_param] = value
return self
aliasdict = AliasSystem(
A=Alias("label"),
B=Alias((0, 10), separator="/"),
C=[Alias("text"), Alias("TL", prefix="+j")]
).update({"C": "abc"})
print(aliasdict)The script output is:
Updating dict
Updating dict
{'A': 'label', 'B': '0/10', 'C': 'abc'}
As you can see, it prints Updating dict twice. One is by the super().__init__ call, another by the AliasSystem().update call. Since update is a built-in method of dict/UserDict, overriding it is not a good idea. I guess we need to change the method name from update() to something like merge()/check()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the AliasSystem class using UserDict in 99845c2.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| """ | ||
| Tests for the alias system. | ||
| """ | ||
|
|
||
| import pytest | ||
| from pygmt.alias import Alias, AliasSystem | ||
| from pygmt.exceptions import GMTInvalidInput | ||
| from pygmt.helpers import build_arg_list | ||
|
|
||
|
|
||
| def func( | ||
| projection=None, | ||
| region=None, | ||
| frame=None, | ||
| label=None, | ||
| text=None, | ||
| offset=None, | ||
| **kwargs, | ||
| ): | ||
| """ | ||
| A simple function to test the alias system. | ||
| """ | ||
| alias = AliasSystem( | ||
| J=Alias(projection, name="projection"), | ||
| R=Alias(region, name="region", separator="/", size=[4, 6]), | ||
| B=Alias(frame, name="frame"), | ||
| U=[ | ||
| Alias(label, name="label"), | ||
| Alias(text, name="text", prefix="+t"), | ||
| Alias(offset, name="offset", prefix="+o", separator="/"), | ||
| ], | ||
| ).update(kwargs) | ||
| return build_arg_list(alias.kwdict) | ||
|
|
||
|
|
||
| def test_alias_system_long_form(): | ||
| """ | ||
| Test that the alias system works with long-form parameters. | ||
| """ | ||
| # One parameter | ||
| assert func(projection="X10c") == ["-JX10c"] | ||
| # Multiple parameters. | ||
| assert func(projection="H10c", region=[0, 10, 0, 20]) == ["-JH10c", "-R0/10/0/20"] | ||
| # Repeatable parameters. | ||
| assert func(frame=["WSen", "xaf", "yaf"]) == ["-BWSen", "-Bxaf", "-Byaf"] | ||
| # Multiple long-form parameters. | ||
| assert func(label="abcd", text="efg", offset=(12, 12)) == ["-Uabcd+tefg+o12/12"] | ||
| assert func( | ||
| projection="H10c", | ||
| region=[0, 10, 0, 20], | ||
| label="abcd", | ||
| text="efg", | ||
| offset=(12, 12), | ||
| frame=["WSen", "xaf", "yaf"], | ||
| ) == ["-BWSen", "-Bxaf", "-Byaf", "-JH10c", "-R0/10/0/20", "-Uabcd+tefg+o12/12"] | ||
|
|
||
|
|
||
| def test_alias_system_one_alias_short_form(): | ||
| """ | ||
| Test that the alias system works when short-form parameters coexist. | ||
| """ | ||
| # Long-form does not exist. | ||
| assert func(A="abc") == ["-Aabc"] | ||
|
|
||
| # Long-form exists but is not given, and short-form is given. | ||
| with pytest.warns( | ||
| SyntaxWarning, | ||
| match="Short-form parameter 'J' is not recommended. Use long-form parameter 'projection' instead.", | ||
| ): | ||
| assert func(J="X10c") == ["-JX10c"] | ||
|
|
||
| # Coexistence of long-form and short-form parameters. | ||
| with pytest.raises( | ||
| GMTInvalidInput, | ||
| match="Short-form parameter 'J' conflicts with long-form parameters and is not recommended. Use long-form parameter 'projection' instead.", | ||
| ): | ||
| func(projection="X10c", J="H10c") | ||
|
|
||
|
|
||
| def test_alias_system_multiple_aliases_short_form(): | ||
| """ | ||
| Test that the alias system works with multiple aliases when short-form parameters | ||
| are used. | ||
| """ | ||
| _msg_long = r"Use long-form parameters 'label', with optional parameters 'text' \(\+t\), 'offset' \(\+o\) instead." | ||
| # Long-form exists but is not given, and short-form is given. | ||
| msg = rf"Short-form parameter 'U' is not recommended. {_msg_long}" | ||
| with pytest.warns(SyntaxWarning, match=msg): | ||
| assert func(U="abcd+tefg") == ["-Uabcd+tefg"] | ||
|
|
||
| # Coexistence of long-form and short-form parameters. | ||
| msg = rf"Short-form parameter 'U' conflicts with long-form parameters and is not recommended. {_msg_long}" | ||
| with pytest.raises(GMTInvalidInput, match=msg): | ||
| func(label="abcd", U="efg") | ||
|
|
||
| with pytest.raises(GMTInvalidInput, match=msg): | ||
| func(text="efg", U="efg") |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should't this be overriding the UserDict's
dataattribute? https://docs.python.org/3/library/collections.html#collections.UserDict.dataThen I suppose you can use
updateas method name? Unless it's still not advised to override that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datais the real dictionary that stores the contents of the UserDict class, so changing the UserDict object also affectsdata.The output is: