Skip to content

Deprecate setting and getting pixels in a WCSAxes Spine - #12621

Closed
dstansby wants to merge 3 commits into
astropy:mainfrom
dstansby:wcs-pix
Closed

Deprecate setting and getting pixels in a WCSAxes Spine#12621
dstansby wants to merge 3 commits into
astropy:mainfrom
dstansby:wcs-pix

Conversation

@dstansby

@dstansby dstansby commented Dec 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

When using Matplotlib, it is generally only safe to calculate pixel coordinates of artists at draw time, as that is when the figure DPI is set in stone. Calculating the pixel coordinates before drawing can lead to issues like #12568.

This PR fixes this issue for WCSAxes Spines by ensuring that pixel calculations are done at draw time. It also deprecates getting and setting pixel coordinates.

This pull request is to partly address #12568. Before that issue can be fully resolved I think there is also a fix required on the Matplotlib side.

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.
  • 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 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

github-actions Bot commented Dec 22, 2021

Copy link
Copy Markdown
Contributor

👋 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?

@dstansby dstansby mentioned this pull request Dec 22, 2021
Closed
@pllim pllim added this to the v5.0.1 milestone Dec 22, 2021
@pllim pllim added API change PRs and issues that change an existing API, possibly requiring a deprecation period and removed backport-v5.0.x labels Dec 22, 2021
@pllim pllim modified the milestones: v5.0.1, v5.1 Dec 22, 2021
@dstansby dstansby changed the title Delay pixel computations in WCSAxes Deprecate setting and getting pixels in a WCSAxes Spine Dec 23, 2021
@dstansby
dstansby marked this pull request as ready for review December 23, 2021 16:19
@pllim
pllim requested a review from Cadair April 21, 2022 21:39
@pllim

pllim commented Apr 21, 2022

Copy link
Copy Markdown
Member

Why half the change log entries point to #12630 ?

@saimn

saimn commented Apr 22, 2022

Copy link
Copy Markdown
Contributor

Moving the milestone to 5.2 since it seems more work is needed (and there are some conflicts).

@saimn saimn modified the milestones: v5.1, v5.2 Apr 22, 2022
@github-actions

github-actions Bot commented May 30, 2022

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label May 30, 2022
@dstansby

dstansby commented May 31, 2022

Copy link
Copy Markdown
Contributor Author

Why half the change log entries point to #12630 ?

Sorry, this depends on #12630, which I've just rebased and taken out of draft. I'll stick this PR in draft until that one is sorted.

@dstansby
dstansby marked this pull request as draft May 31, 2022 08:43
@pllim pllim removed the Close? Tell stale bot that this issue/PR is stale label May 31, 2022
@pllim

pllim commented May 31, 2022

Copy link
Copy Markdown
Member

If you rebase anyway, it will give you a bit more time before the bot complains again. FYI.

@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Jun 1, 2022
@github-actions

github-actions Bot commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 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.

@github-actions github-actions Bot added the closed-by-bot Closed by stale bot label Jul 3, 2022
@github-actions

github-actions Bot commented Jul 3, 2022

Copy link
Copy Markdown
Contributor

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

@github-actions github-actions Bot closed this Jul 3, 2022
@dstansby dstansby mentioned this pull request Nov 6, 2022
Merged
9 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cadair Cadair Awaiting requested review from Cadair
@astrofrog astrofrog Awaiting requested review from astrofrog astrofrog will be requested when the pull request is marked ready for review astrofrog is a code owner
@larrybradley larrybradley Awaiting requested review from larrybradley larrybradley will be requested when the pull request is marked ready for review larrybradley is a code owner

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period Bug Close? Tell stale bot that this issue/PR is stale closed-by-bot Closed by stale bot visualization.wcsaxes

Projects

None yet

Milestone

v5.2

Development

Successfully merging this pull request may close these issues.

3 participants

@dstansby @pllim @saimn