-
Notifications
You must be signed in to change notification settings - Fork 18
[Feature add] Share Axes in GeoPlot + bug fixes #159
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
[Feature add] Share Axes in GeoPlot + bug fixes #159
Conversation
|
Note: Tests will fail on added tests. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 adds a feature to share axes on geographic plots and includes bug fixes related to axis sharing and test configuration.
- Adds tests for validating shared geographic tick configurations.
- Refactors axis sharing logic in GeoAxes and related modules.
- Removes extraneous debug prints in test configuration.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ultraplot/tests/test_geographic.py | Adds new tests for geographic ticks and shared axis behavior |
| ultraplot/tests/conftest.py | Removes debug print statements from test failure handling |
| ultraplot/figure.py | Updates subplot creation and axis sharing logic for GeoAxes |
| ultraplot/axes/shared.py | Adjusts share function signatures and minor ticker handling |
| ultraplot/axes/geo.py | Extensive refactor for GeoAxes: sharing, toggling ticks and gridliner labels |
| ultraplot/axes/base.py | Adds an _unshare method and a guard for NaN padding values |
Comments suppressed due to low confidence (1)
ultraplot/axes/geo.py:1780
- [nitpick] Consider renaming the loop variable 'object' to something like 'obj' to avoid shadowing the built-in 'object'.
for object in objects:
|
Will add more tests tomorrow for codecov but it's looking good locally with a few plots showing different sizes but otherwise have high visual fidelity. Opening this up for review. |
beckermr
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.
I took a closer look at the image test failure:
Here is the code:
@pytest.mark.mpl_image_compare
def test_geoticks():
lonlim = (-140, 60)
latlim = (-10, 50)
basemap_projection = uplt.Proj(
"cyl",
lonlim=lonlim,
latlim=latlim,
backend="basemap",
)
fig, ax = uplt.subplots(
ncols=3,
proj=(
"cyl", # cartopy
"cyl", # cartopy
basemap_projection, # basemap
),
share=0,
)
settings = dict(land=True, labels=True, lonlines=20, latlines=20)
# Shows sensible "default"; uses cartopy backend to show the grid lines with ticks
ax[0].format(
lonlim=lonlim,
latlim=latlim,
**settings,
)
# Add lateral ticks only
ax[1].format(
latticklen=True,
gridminor=True,
lonlim=lonlim,
latlim=latlim,
**settings,
)
ax[2].format(
latticklen=5.0,
lonticklen=2.0,
grid=False,
gridminor=False,
**settings,
)
return fig
Here is a zoom of the middle panel (top is baseline, bottom is this PR):
It appears that the call to set latlim for the middle panel is now being ignored in the new version of the code in this PR. This is definitely a bug. I don't see why latlim should now be ignored.
|
Fixed @beckermr : |
|
Not sure why the ticks are not using the full range but I cut off the edges here and it looks the same. |
|
weirdly enough when I do this for the basemap the yticks behave strangely. Only one appears. |
beckermr
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.
Two more items. Thank you so much for the good work on this PR!
beckermr
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.
LGTM! Great work on this PR!
|
Will draft an appropriate release to 1.5. Lots of new features 😄 |
- Added ability to share GeoAxes - Fixed numerous bugs regarding ticks on GeoAxes - Backend allows for sharing and unsharing axes. --------- Co-authored-by: Matthew R. Becker <beckermr@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Addresses #155 and #158