Refactor GeoAxes gridliner handling and format flow; remove cartopy monkey patches#454
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the GeoAxes gridliner handling to eliminate cartopy monkey-patching in favor of local subclasses and introduces a unified adapter pattern for both cartopy and basemap backends. The core changes improve code maintainability and extensibility by creating clear abstraction boundaries.
Changes:
- Replaced cartopy monkey-patching with
_CartopyGridlinerand_CartopyLabelsubclasses - Introduced
_GridlinerAdapterprotocol with backend-specific implementations (_CartopyGridlinerAdapterand_BasemapGridlinerAdapter) - Split the large
GeoAxes.format()method into focused helper methods for improved clarity - Added comprehensive type hints throughout the geographic axes implementation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ultraplot/tests/test_geographic.py | Added comprehensive tests for gridliner label handling, adapter refresh, tick positions, and tick_params updates across both cartopy and basemap backends |
| ultraplot/axes/geo.py | Major refactoring: introduced adapter pattern, removed monkey patches, split format() into focused helpers, added type hints, and unified backend handling |
Comments suppressed due to low confidence (4)
ultraplot/axes/geo.py:3141
- For loop variable 'obj' is not used in the loop body.
for obj in feat:
ultraplot/axes/geo.py:24
- Import of 'mtransforms' is not used.
import matplotlib.transforms as mtransforms
ultraplot/axes/geo.py:29
- Import of 'pticker' is not used.
from .. import ticker as pticker
ultraplot/axes/geo.py:39
- Import of 'ic' is not used.
from ..internals import (
_not_none,
_pop_rc,
_version_cartopy,
docstring,
ic, # noqa: F401
labels,
warnings,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1469,7 +2336,7 @@ class _CartopyAxes(GeoAxes, _GeoAxes): | |||
| # NOTE: The rename argument wrapper belongs here instead of format() because | |||
| # these arguments were previously only accepted during initialization. | |||
| @warnings._rename_kwargs("0.10", circular="round", autoextent="extent") | |||
| def __init__(self, *args, map_projection=None, **kwargs): | |||
| def __init__(self, *args: Any, map_projection: Any = None, **kwargs: Any) -> None: | |||
There was a problem hiding this comment.
This initialization method calls GeoAxes.init multiple times, via this call and this call.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
This comment was marked as resolved.
This comment was marked as resolved.
Bad copilot, you messed things up |
cvanelteren
force-pushed
the
refactor/geo-gridliner-adapters
branch
from
c6f6a2b to
8952108
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…onkey patches (#454) * Refactor geo gridliner adapters * Add adapter-focused geographic tests * Refine geo gridliner helpers and constants * Add gridliner adapter tests * Refactor cartopy gridliner overrides * Tweak gridliner subclass wording * Refactor GeoAxes.format into helpers * Document GeoAxes.format flow * Restore gridliner toggle call for empty labels
Summary
_CartopyGridliner/_CartopyLabelsubclasses and agridlines()override.GeoAxes.formatinto focused helpers for clarity and extensibility; add format flow comment.test_cartesian_and_geo).User API is the same.