Repository navigation
Conversation
|
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
4af2e7e to
56b0ecd
Compare
ef21285 to
9a4dc77
Compare
mhvk
left a comment
There was a problem hiding this comment.
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.
That is basically exactly what the APE 14 high-level API does. Using |
|
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. |
|
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. |
|
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'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. |
@mhvk suggested in #18338 having a way to globally set the default
preserve_unitsbehavior, so this takes a first stab at it.I took the approach of using a
ScienceStatebecause my understanding is thatScienceStateshould 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_unitstoFalsetoTruewill lead to numerical values e.g. fromwcs.all_pix2worldactually changing.Anyway, this is open for discussion, and should not be merged at this point.