Repository navigation
Calculate WCSAxes TickLabel pixel coords at draw time - #12630
Conversation
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I've rebased this, and I think it's ready for review. |
pllim
added
Refactoring
and removed
Close?
| if (axis is None or | ||
| world is None or | ||
| angle is None or | ||
| text is None or | ||
| axis_displacement is None or | ||
| data is None): | ||
| raise ValueError(f'All of the following arguments must be provided: {required_args}') |
There was a problem hiding this comment.
If a function is missing a required argument then that should result in a TypeError, not a ValueError. Furthermore, given that the function already defines required_args it is not necessary to list them again in the if-statement, and we can make the error message more helpful too:
| if (axis is None or | |
| world is None or | |
| angle is None or | |
| text is None or | |
| axis_displacement is None or | |
| data is None): | |
| raise ValueError(f'All of the following arguments must be provided: {required_args}') | |
| if missing_args := [arg for arg in required_args if locals()[arg] is None]: | |
| raise TypeError(f'The following mandatory arguments are missing: {missing_args}') |
There was a problem hiding this comment.
I'll need to apply this suggestion manually since it conflicts
There was a problem hiding this comment.
This suggestion doesn't work right as-is as locals() doesn't always contain the various arguments, so in the interest of time I'll just apply the change in exception type.
|
Hello @dstansby 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style. There are no PEP8 style issues with this pull request - thanks! 🎉 Comment last updated at 2022-09-08 16:22:10 UTC |
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
Looks good to me now, once CI passes.
I do wonder if we should deprecate the fact that classes like TickLabels are public - they were never really meant to be public API.
|
The failing figure test means we will probably need to update the reference images, I can try and do this week. |
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
|
|
||
|
|
||
| @pytest.mark.remote_data(source='astropy') | ||
| @pytest.mark.remote_data |
There was a problem hiding this comment.
This is the other useful thing about having a wrapper decorator, you can make one decorator do remote data and mpl_image_compare ;)
|
I checked the test failure and it was a very minor change in tick labels so I've updated the hashes. |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
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 If these instructions are inaccurate, feel free to suggest an improvement. |
…630-on-v5.1.x Backport PR #12630 on branch v5.1.x (Calculate WCSAxes TickLabel pixel coords at draw time)
(cherry picked from commit 47dc553)
(cherry picked from commit 47dc553)
…-v5.0.x Backport PR #12630 on branch v5.0.x (Calculate WCSAxes TickLabel pixel coords at draw time)
6 participants
Description
This is part of solving #12568. Instead of calculating tick and tick-label coordinates before draw time, which in general is not a safe thing to do, calculate them at draw time. This involves not using the
pixelargument toadd()- I think I've found the best way to deprecate this in the least API breaking way?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.
Extra CIlabel.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.