-
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
Conversation
4c2cd8c to
f74919e
Compare
f74919e to
410b433
Compare
70f883c to
95af3d5
Compare
95af3d5 to
ddcc7b8
Compare
pygmt/src/basemap.py
Outdated
| basemap - Plot base maps and frames. | ||
| """ | ||
|
|
||
| from pygmt.alias import Alias, 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.
Changes in basemap.py is only meant for proof of concept. I plan to revert the changes in basemap.py and open separate PRs for it.
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.
Pull Request Overview
This PR implements the AliasSystem class to provide a new alias system that can coexist with the existing system, enabling more Pythonic parameter handling by building GMT options from multiple PyGMT parameters without abusing kwargs.
Key changes:
- Implements the
AliasSystemclass with validation for short/long-form parameter conflicts - Adds comprehensive test coverage for the new alias system functionality
- Updates the
basemapfunction to demonstrate usage of the new alias system
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pygmt/alias.py |
Implements the core AliasSystem class with parameter conflict detection and warnings |
pygmt/tests/test_alias_system.py |
Adds comprehensive tests for long-form, short-form, and conflict scenarios |
pygmt/src/basemap.py |
Updates basemap function to use the new alias system for region, projection, and frame parameters |
pygmt/src/basemap.py
Outdated
| alias = AliasSystem( | ||
| R=Alias(region, name="region", separator="/", size=[4, 6]), | ||
| J=Alias(projection, name="projection"), | ||
| B=Alias(frame, name="frame"), | ||
| ).update(kwargs) |
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.
Actually, I'm still debating if we should change these lines to:
| 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.
| Initialize the alias system as a dictionary with current parameter values. | ||
| """ | ||
| # Store the aliases in a dictionary, to be used in the merge() method. | ||
| self.aliasdict = kwargs |
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 data attribute? https://docs.python.org/3/library/collections.html#collections.UserDict.data
| self.aliasdict = kwargs | |
| self.data = kwargs |
Then I suppose you can use update as 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.
data is the real dictionary that stores the contents of the UserDict class, so changing the UserDict object also affects data.
from collections import UserDict
alias = UserDict(A="label", B="text")
print(alias.data)
print(alias)
alias.data = {"C": True}
print(alias.data)
print(alias)
alias.update({"D": "good"})
print(alias.data)
print(alias)The output is:
{'A': 'label', 'B': 'text'}
{'A': 'label', 'B': 'text'}
{'C': True}
{'C': True}
{'C': True, 'D': 'good'}
{'C': True, 'D': 'good'}
weiji14
left a comment
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.
Don't want to hold this up since I'll be busy over the next few weeks (conference + workshops), so I think ok to merge this so the work on the new Alias system can advance. Thanks again @seisman!
…iasSystem/aliassystem
This PR implements the
AliasSystemclass for the new alias system proposed in #3239.As mentioned in #3239, the new alias system has the following pros and cons:
pros:
kwargsanymoreCons:
{aliases}in docstrings is not supported in the new alias system, since we can't access the local variables in a decorator. So we need to manually edit the{aliases}in docstrings.