Skip to content

New Alias System: Add the AliasSystem class - #4000

Merged
seisman merged 30 commits into
mainfrom
AliasSystem/aliassystem
Jul 29, 2025
Merged

New Alias System: Add the AliasSystem class#4000
seisman merged 30 commits into
mainfrom
AliasSystem/aliassystem

Conversation

@seisman

@seismanseisman commented Jul 16, 2025

Copy link
Copy Markdown
Member

This PR implements the AliasSystem class for the new alias system proposed in #3239.

As mentioned in #3239, the new alias system has the following pros and cons:

pros:

Cons:

  • Big refactors may introduce new bugs. [We can always fix them if any.]
  • The placeholder {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.

@seisman
seismanforce-pushed the AliasSystem/aliassystem branch 3 times, most recently from 4c2cd8c to f74919eCompareJuly 16, 2025 03:01
@seisman
seismanforce-pushed the AliasSystem/aliassystem branch from f74919e to 410b433CompareJuly 21, 2025 10:54
@seisman
seisman changed the base branch from main to AliasSystem/aliasJuly 21, 2025 10:55
@seismanseisman added the enhancement Improving an existing feature label Jul 21, 2025
@seismanseisman added this to the 0.17.0 milestone Jul 21, 2025
@seisman
seismanforce-pushed the AliasSystem/aliassystem branch from 70f883c to 95af3d5CompareJuly 21, 2025 12:43
@seisman
seismanforce-pushed the AliasSystem/aliassystem branch from 95af3d5 to ddcc7b8CompareJuly 21, 2025 13:02
@seisman
seisman marked this pull request as ready for review July 22, 2025 05:47
@seismanseisman added the needs review This PR has higher priority and needs review. label Jul 22, 2025
Comment threadpygmt/src/basemap.py Outdated
basemap - Plot base maps and frames.
"""

from pygmt.alias import Alias, AliasSystem

Copy link
Copy Markdown
MemberAuthor

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.

@seisman
seisman requested a review from CopilotJuly 22, 2025 06:16

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 AliasSystem class with validation for short/long-form parameter conflicts
  • Adds comprehensive test coverage for the new alias system functionality
  • Updates the basemap function 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.

FileDescription
pygmt/alias.pyImplements the core AliasSystem class with parameter conflict detection and warnings
pygmt/tests/test_alias_system.pyAdds comprehensive tests for long-form, short-form, and conflict scenarios
pygmt/src/basemap.pyUpdates basemap function to use the new alias system for region, projection, and frame parameters

Comment threadpygmt/alias.py Outdated
Comment threadpygmt/alias.py Outdated
Comment threadpygmt/alias.py Outdated
Comment threadpygmt/src/basemap.py Outdated
Comment on lines +85 to +89
alias = AliasSystem(
R=Alias(region, name="region", separator="/", size=[4, 6]),
J=Alias(projection, name="projection"),
B=Alias(frame, name="frame"),
).update(kwargs)

Copy link
Copy Markdown
MemberAuthor

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:

Suggested change
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.

@weiji14weiji14Jul 24, 2025

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 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.

Copy link
Copy Markdown
MemberAuthor

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.

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.

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.

or rename alias to kwdict, but both may be confusing.

how about aliasdict = AliasSystem(...)?

@seismanseismanJul 25, 2025

Copy link
Copy Markdown
MemberAuthor

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:

fromcollectionsimportUserDictfromcollections.abcimportSequencefrompygmt.aliasimportAliasclassAliasSystem(UserDict):
def__init__(self, **kwargs):
self.aliasdict=kwargskwdict= {}
foroption, aliasesinkwargs.items():
ifisinstance(aliases, Sequence):
values= [alias._valueforaliasinaliasesifalias._valueisnotNone]
ifvalues:
kwdict[option] ="".join(values)
elifaliases._valueisnotNone:
kwdict[option] =aliases._valuesuper().__init__(kwdict)
defupdate(self, kwargs):
print("Updating dict")
# Add more checks later.forshort_param, valueinkwargs.items():
self[short_param] =valuereturnselfaliasdict=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()?

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadpygmt/alias.py
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

@weiji14weiji14Jul 25, 2025

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.

Should't this be overriding the UserDict's data attribute? https://docs.python.org/3/library/collections.html#collections.UserDict.data

Suggested change
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.

Copy link
Copy Markdown
MemberAuthor

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.

fromcollectionsimportUserDictalias=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'}

@weiji14weiji14 left a comment

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.

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!

@seismanseisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Jul 29, 2025
Base automatically changed from AliasSystem/alias to mainJuly 29, 2025 03:52
@seismanseisman removed the final review call This PR requires final review and approval from a second reviewer label Jul 29, 2025
@seisman
seisman merged commit 170f82e into mainJul 29, 2025
23 of 24 checks passed
@seisman
seisman deleted the AliasSystem/aliassystem branch July 29, 2025 04:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementImproving an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@seisman@weiji14