Skip to content

GeoTicks not responsive #253

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

Merged
merged 19 commits into from
Jun 10, 2025
Merged

Conversation

cvanelteren
Copy link
Contributor

Addresses #252

image

# %%
import ultraplot as uplt, numpy as np, pandas as pd
import numpy as np
import matplotlib.patheffects as pe

fig, ax = uplt.subplots(proj="cyl")
ax.format(
    latlabels="l",
    lonlabels="b",
)
uplt.show(block=1)

@cvanelteren cvanelteren marked this pull request as draft June 9, 2025 06:36
@cvanelteren
Copy link
Contributor Author

cvanelteren commented Jun 9, 2025

This PR combines partly the changes from #244. The difference between these tow PRs is that this just targets GEO whereas #244 also combines this with more profound axis sharing which I will target at a later date.

Copy link

codecov bot commented Jun 9, 2025

Codecov Report

Attention: Patch coverage is 92.94872% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ultraplot/axes/geo.py 88.31% 4 Missing and 5 partials ⚠️
ultraplot/figure.py 92.59% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@cvanelteren cvanelteren marked this pull request as ready for review June 9, 2025 12:18
@cvanelteren
Copy link
Contributor Author

Looks good. The failure of the test is expected given the changes.

@cvanelteren cvanelteren requested a review from Copilot June 9, 2025 12:18
Copy link
Contributor

@Copilot Copilot AI left a 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 refactors how geographic tick labels are toggled and shared across subplots and backends, and updates tests to match the new API.

  • Renames gridliner label toggles to label{side} and adds _is_ticklabel_on helpers.
  • Introduces _share_labels_with_others in Figure to centralize label sharing logic.
  • Updates and adds tests (test_tick_toggler, test_sharing_cartopy_with_colorbar) to cover the new behavior.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ultraplot/tests/test_geographic.py Adjusted tests for new label{side} API, added tick toggler and colorbar sharing tests
ultraplot/figure.py Added _share_labels_with_others and integrated it into format flow
ultraplot/axes/geo.py Refactored _toggle_gridliner_labels, added _is_ticklabel_on, updated gridline updates
ultraplot/axes/base.py Added _is_ticklabel_on for non-geographic axes
Comments suppressed due to low confidence (4)

ultraplot/axes/geo.py:1750

  • The ordering of sides here differs from the later zip in update_major_gridlines (which uses labelleft labelright labeltop labelbottom geo), potentially causing mismatched label toggles.
for side, lon, lat in zip("labelleft labelright labelbottom labeltop geo".split(), lonarray, latarray):

ultraplot/axes/geo.py:2199

  • Inconsistent side ordering compared to earlier implementation may lead to incorrect behavior—align the side lists.
for side, lonon, laton in zip("labelleft labelright labeltop labelbottom geo".split(), lonarray, latarray):

ultraplot/axes/base.py:3202

  • [nitpick] Docstring could clarify which Matplotlib attributes (label1 vs label2) map to which sides and what each represents.
def _is_ticklabel_on(self, side: str) -> bool:

ultraplot/figure.py:1317

  • Debug print left in _share_labels_with_others should be removed or replaced with a logger call to avoid console noise.
print(axi.number)

@cvanelteren cvanelteren requested a review from beckermr June 9, 2025 12:30
@cvanelteren cvanelteren requested a review from beckermr June 10, 2025 07:01
@beckermr
Copy link
Collaborator

testing error is expected and new plot looks better

@beckermr beckermr merged commit c8bcb9e into Ultraplot:main Jun 10, 2025
11 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants