Skip to content

Added global default_preserve_units option - #18463

Closed
astrofrog wants to merge 2 commits into
astropy:mainfrom
astrofrog:preserve-units-config
Closed

astrofrog wants to merge 2 commits into
astropy:mainfrom
astrofrog:preserve-units-config

Conversation

@astrofrog

Copy link
Copy Markdown
Member

@mhvk suggested in #18338 having a way to globally set the default preserve_units behavior, so this takes a first stab at it.

I took the approach of using a ScienceState because my understanding is that ScienceState should be used for things that can actually change the results of a chunk of code, whereas configuration items are more for things that change how the code is run or any displayed information. However, I'm happy to discuss if my understanding is not correct.

Here changing from preserve_units to False to True will lead to numerical values e.g. from wcs.all_pix2world actually changing.

Anyway, this is open for discussion, and should not be merged at this point.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • 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 instructions for rebase and squash.
  • 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?
  • 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
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?

@astrofrog
astrofrog force-pushed the preserve-units-config branch from 4af2e7e to 56b0ecd Compare July 25, 2025 13:48
@pllim pllim modified the milestones: v7.2.0, v8.0.0 Jul 25, 2025
@astrofrog
astrofrog force-pushed the preserve-units-config branch from ef21285 to 9a4dc77 Compare July 26, 2025 20:41

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you are right that this has to a ScienceState given that e.g. all_pix2world results indeed change. Given that, this is probably the right approach.

But this makes me wonder more generally: shouldn't there be an API change / replacement API that just uses Quantity for all world coordinates? Not having to worry about (hidden) units is exactly what Quantity is meant to solve...

One way that may avoid defining entirely new functions would be for the existing API to start ensuring that if you put Quantity in (e.g., in units of pix), you'll get Quantity out.

@astrofrog

Copy link
Copy Markdown
Member Author

But this makes me wonder more generally: shouldn't there be an API change / replacement API that just uses Quantity for all world coordinates? Not having to worry about (hidden) units is exactly what Quantity is meant to solve...

That is basically exactly what the APE 14 high-level API does. Using wcs.pixel_to_world will return Quantities, SkyCoords, Time objects and so on. Now that APE 14 is firmly established, we may want to re-work the WCS documentation to make that API much more prominent so that new users gravitate towards that. I don't think we can easily change e.g. all_pix2world without breaking a lot of code.

@pllim

pllim commented Jul 31, 2025

Copy link
Copy Markdown
Member

Global switch might be painful for downstream if they want to do gradual adoption and have a lot of different components on different dev cycles.

@astrofrog

Copy link
Copy Markdown
Member Author

Well to be clear this is an opt in thing that individual users can choose to use but we would definitely need to add a note that packages that use WCS aren't guaranteed to work with this option set to True. Another option is to make this ScienceState private for now so we can start testing things but not letting users use it.

@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Dec 24, 2025
@github-actions

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 closed-by-bot Closed by stale bot label Jan 24, 2026
@github-actions

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 Jan 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Close? Tell stale bot that this issue/PR is stale closed-by-bot Closed by stale bot needs-discussion no-changelog-entry-needed wcs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants