Skip to content
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

Add functionality for correct transforms in ax.annotate #2065

Merged
merged 14 commits into from
Sep 2, 2022

Conversation

abrammer
Copy link
Contributor

@abrammer abrammer commented Aug 4, 2022

fixes #1720

Rationale

Annotate has been "broken" for quite some time (https://stackoverflow.com/questions/25416600/why-the-annotate-worked-unexpected-here-in-cartopy), and while the work around is somewhat simple, fixing it to be more intuitive also seems fairly straight forward. Also recently raised in the open issue #1720 which spurred me on to open this PR.

I'm not particularly aware of underlying stuff here or if this is the "correct" way to fix it, but it seems like a relatively simple fix.

Implications

ax.annotate will function like most other GeoAxes.methods, but previous work arounds will continue to work.

Examples

added a couple simple tests which exemplify the various annotate transform configurations.

Example annotated maps:
annotate_homolosine
annotate_mercator

lib/cartopy/mpl/geoaxes.py Show resolved Hide resolved
lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks like a reasonable approach.

For the tests, I would suggest combining them all into one with the different cases pointing to different positions, or parameterizing over the transforms with pytest.mark.parametrize(), I'm not sure we need a lot of projections to be tested, more so the points/transforms being moved around.

One thing I don't see is the explicit "data" case? If I'm following correctly, that should be the same as the transform passed into the annotate call, and if there is none, default to the map projection.

It might be good to also test that supplying different transforms for xycoords and textcoords works as expected too, and we aren't accidentally only using one of them.

lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
abrammer and others added 4 commits August 21, 2022 10:56
change wording in test docstring

Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
fix comment typo

Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
@abrammer
Copy link
Contributor Author

abrammer commented Aug 21, 2022

@greglucas Thanks for the input. Consolidated the tests and added a couple permutations of providing different CRS transforms to xycoords and textcoords. Parameterized a couple base projections, not sure if that's really needed but it's also pretty simple.

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just a few minor comments. I like the combinations you chose for the tests.

lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just one minor comment left about removing the parametrize now that we're only using one projection (sorry about sending you down that path!).

lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_mpl_integration.py Show resolved Hide resolved
@abrammer
Copy link
Contributor Author

abrammer commented Sep 1, 2022

yeah, no worries. It was a simple addition and a simple removal.

@greglucas greglucas merged commit 8c31a15 into SciTools:main Sep 2, 2022
@greglucas
Copy link
Contributor

Thank you for contributing this, @abrammer! Hope to see you around again.

@QuLogic QuLogic added this to the 0.21 milestone Sep 2, 2022
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.

annotate
4 participants