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

Allow zero WCSAxes ticks #14160

Merged
merged 6 commits into from Apr 21, 2023
Merged

Allow zero WCSAxes ticks #14160

merged 6 commits into from Apr 21, 2023

Conversation

dstansby
Copy link
Contributor

@dstansby dstansby commented Dec 9, 2022

Fixes #14159

Description

This pull request is to address ...

Fixes #

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@github-actions
Copy link

github-actions bot commented Dec 9, 2022

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@pllim pllim added this to the v5.3 milestone Dec 9, 2022
@pllim pllim added the Bug label Dec 9, 2022
@dstansby dstansby marked this pull request as ready for review December 10, 2022 10:07
@dhomeier
Copy link
Contributor

Thanks for the PR. There is a new failure with matplotlib dev, not introduced by these changes but probably rather an upstream change to TickLabels.draw() in 3.7.0dev, or perhaps rather to the underlying backend_agg.RendererAgg.draw() – wondering if you had any insights into that.

@dstansby
Copy link
Contributor Author

🤷 - if you want to tag me in an issue for those new failures I might be able to take a look at them at some point.

@dhomeier
Copy link
Contributor

🤷 - if you want to tag me in an issue for those new failures I might be able to take a look at them at some point.

#14174

@dstansby dstansby requested review from dhomeier and removed request for astrofrog, Cadair and larrybradley December 14, 2022 20:58
@pllim
Copy link
Member

pllim commented Dec 23, 2022

CI should be green on main again. Please rebase. Thanks!

@astrofrog
Copy link
Member

Looks like the hash is missing for Matplotlib 3.2.2?

@astrofrog
Copy link
Member

I've updated the hash for 3.2.2 and rebased

Copy link
Member

@astrofrog astrofrog 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 know if the CI is happy!

@astrofrog astrofrog added 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.2.x on-merge: backport to v5.2.x labels Mar 30, 2023
@astrofrog astrofrog modified the milestones: v5.3, v5.0.7 Mar 30, 2023
@astrofrog astrofrog added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Mar 30, 2023
@pllim
Copy link
Member

pllim commented Mar 30, 2023

I am not sure if this will backport cleanly because we bumped matplotlib minversion on main. Can we not backport?

@astrofrog
Copy link
Member

I would like to backport

@pllim pllim removed the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Apr 18, 2023
@pllim
Copy link
Member

pllim commented Apr 18, 2023

This now also needs a rebase. We dropped mpl311 and mpl322. It is mpl334 now.

@larrybradley
Copy link
Member

@pllim I rebased and fixed the changelog.

@@ -21,6 +21,7 @@
"astropy.visualization.wcsaxes.tests.test_images.TestBasic.test_changed_axis_units": "056ec7123c67fa7ee6269bec47641bfe4932630a79d02029e71164b522fadacd",
"astropy.visualization.wcsaxes.tests.test_images.TestBasic.test_minor_ticks": "5224f8fa725901ff74a4558987ceaae0363f98894d2c6a12d8c557db500ff7fe",
"astropy.visualization.wcsaxes.tests.test_images.TestBasic.test_ticks_labels": "f4944296c37a6e8ea9d30126709da3567a25908c1665e8cad10e894dd87cd54d",
"astropy.visualization.wcsaxes.tests.test_images.TestBasic.test_no_ticks": "14c20201942f1db9796db8747ffcdec77254d79a9ae7dc45ce668ebf07c6de8e",
Copy link
Member

Choose a reason for hiding this comment

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

CircleCI failed with Baseline hash not found but can we ignore it?

@astrofrog
Copy link
Member

@pllim - no the issue is that we are missing an update to the mpl334 file. If no one gets to it by later today I can try and fix that.

@pllim
Copy link
Member

pllim commented Apr 19, 2023

I don't know exactly what that means, so I'll leave it to you then; thanks, @astrofrog !

@dhomeier
Copy link
Contributor

Some mismatches with the hashes:

__________________________ TestBasic.test_tick_angles __________________________
Hash 475f028bd48a51fe9dfc07fe3d58e63e629511fffeb3a4db27a26f6427b4256b doesn't match hash e828e41d48899dd706cc58bb6fb93427d30b4c16ccd360813b391a880bee692c in library /home/circleci/project/astropy/tests/figures/py39-test-image-mpl334-cov.json for test astropy.visualization.wcsaxes.tests.test_images.TestBasic.test_tick_angles.

Image comparison test
---------------------
The comparison to the baseline image succeeded.
__________________ TestBasic.test_tick_angles_non_square_axes __________________
Hash 3a3a15f71e7303368bd7cebb70c6fd1f9d6710edf8af2ceb68642c64c74492a2 doesn't match hash adc79cf700df608e662bd9b4904fd8160dce15e67d04698c04f650808411ea97 in library /home/circleci/project/astropy/tests/figures/py39-test-image-mpl334-cov.json for test astropy.visualization.wcsaxes.tests.test_images.TestBasic.test_tick_angles_non_square_axes.

Image comparison test
---------------------
The comparison to the baseline image succeeded.
___________________________ test_tickable_gridlines ____________________________
Hash 1be26963aaba221f9cd396aea0b9618e05ab8a49f8c02c0b4540041e91d06ce7 doesn't match hash ba289534a6a65f01d4dbe050bdcab5e97f19549339896dde84e3b861cd1b07ba in library /home/circleci/project/astropy/tests/figures/py39-test-image-mpl334-cov.json for test astropy.visualization.wcsaxes.tests.test_images.test_tickable_gridlines.

Image comparison test
---------------------
The comparison to the baseline image succeeded.
__________________ TestTransformCoordMeta.test_coords_overlay __________________
Hash f84074e12142676d17aca8e907065cf1eec4a55ea7d60ce7d7f22f456c09c24c doesn't match hash 673e604c362ea09ff027da86b5c7f39bf41469dc92143b382010144ab5779330 in library /home/circleci/project/astropy/tests/figures/py39-test-image-mpl334-cov.json for test astropy.visualization.wcsaxes.tests.test_transform_coord_meta.TestTransformCoordMeta.test_coords_overlay.

Image comparison test
---------------------
The comparison to the baseline image succeeded.
___________________ TestTransformCoordMeta.test_direct_init ____________________
Hash 99e84ea7ae80a9444c73d4fb9f24d707795149e1aeb18c3c1a7072bc3e134732 doesn't match hash 3d7c5f4e7a2e9025b9f05001cd41b24a3f3a09420adb5ea2241dd82188cccff2 in library /home/circleci/project/astropy/tests/figures/py39-test-image-mpl334-cov.json for test astropy.visualization.wcsaxes.tests.test_transform_coord_meta.TestTransformCoordMeta.test_direct_init.

@larrybradley
Copy link
Member

I added the mpl hash and tests pass now.

@larrybradley larrybradley merged commit fb8d854 into astropy:main Apr 21, 2023
23 of 25 checks passed
@lumberbot-app
Copy link

lumberbot-app bot commented Apr 21, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v5.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 fb8d85481b87ffe10fb94bf7b8d55f35877dacec
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #14160: Allow zero WCSAxes ticks'
  1. Push to a named branch:
git push YOURFORK v5.0.x:auto-backport-of-pr-14160-on-v5.0.x
  1. Create a PR against branch v5.0.x, I would have named this PR:

"Backport PR #14160 on branch v5.0.x (Allow zero WCSAxes ticks)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app
Copy link

lumberbot-app bot commented Apr 21, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v5.2.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 fb8d85481b87ffe10fb94bf7b8d55f35877dacec
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #14160: Allow zero WCSAxes ticks'
  1. Push to a named branch:
git push YOURFORK v5.2.x:auto-backport-of-pr-14160-on-v5.2.x
  1. Create a PR against branch v5.2.x, I would have named this PR:

"Backport PR #14160 on branch v5.2.x (Allow zero WCSAxes ticks)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@pllim
Copy link
Member

pllim commented Apr 21, 2023

Do we have to backport?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wcsaxes crashes when trying to set number of ticks to 0
5 participants