-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
APE 14: pixel_to_world_values ignores CUNIT #16432
Comments
The WCS class converts units to SI (this is actually done by WCSLIB behind the scenes). If you try and print out the WCS you will probably see it is in m. This is also a pet peeve of @Cadair's as WCS converts arcsec to degrees. This isn't technically a bug as we know about it and it is a limitation of WCSLIB. That's not to say we can't try and solve this though - perhaps we could save the original units in a private attribute in the WCS class and then expose the original units via the APE 14 API and deal with conversions behind the scenes. |
Yes this is super frustrating. It would also be nice if there was a way to undo this conversion on the result of WCS.to_header(). |
Agree. Given that the APE 14 is built on top of WCSLIB, it should be trivial to report the correct units and do the conversion in the wrappers, and more importantly give consistent behavior between If my header defines CUNITn, I would expect to get the same CUNITn back with the APE 14 interface. Getting back a unit other than what is in CUNITn has to be a bug, as there's no way to predict it. If my unit is optical depth in a Parma cheese rind, I don't expect that to get that value converted to an SI unit. I expect to get back units of cheese rind depth. |
The only question is do we consider this a breaking API change or a bugfix? |
Perhaps we should first try and implement it as opt-in via a kwarg and then decide later if we want to make it the default via a deprecation period? Having it opt-in would allow us to iron out any bugs anyway. |
Yep! |
Are they duplicate issues? |
Not duplicate issues, as each issue shows how different methods suffer from the similar issues of internal conversion to specific units and in the case of #3658, how the FITS headers describing the WCS don't round-trip properly because of this internal conversion. I would argue that APE 14 is a new API, and it specifically uses quantities, so it should respect quantities and the quantity values explicitly. It does not, which is a bug. Agree that an implementation first would be nice, and then we can decide whether its a bug or a new feature (and for which methods), and whether it just gets implemented for the APE 14 interface or for all the methods. |
One slight complication is that e.g. I have reached out to Mark to get his input on the matter before we proceed with anything on our side (in case it's possible for him to add an option to not convert). Rather than just change the APE 14 API, there are two alternatives I wanted to mention:
|
I am pro a new class, though it's probably a lot of work. |
Yes, a lot of work.
Agree this is the way to go for now. And do this for the APE 14 interface, as at least with that interface, people are already thinking about units and want units to round trip. And there's less legacy code using this interface, so deprecations should have lower impact. The opt-in behavior might be something like a FWIW, here's another manifestation of the same problem from my example above: In [59]: hdulist['SCI'].header['CUNIT3']
Out[59]: 'um'
In [60]: wcs.world_axis_units
Out[60]: ['deg', 'deg', 'm'] Again, that's the APE 14 interface. One could conceivable extend this to |
Description
When reading in spectral coordinates, the
WCS.pixel_to_world_values()
method reports the value in different units than are specified in CUNIT for that axis. An example using JWST MIRI MRS data:As you can see, CUNIT3 is in microns, but the value returned is in meters.
Expected behavior
I would expect that the
pixel_to_world_values
would report values in units specified by CUNITn. This is whatgwcs.WCS
does for the same file. First,pip install jwst
, then:Here it is returned in microns, as expected.
How to Reproduce
See above.
Versions
The text was updated successfully, but these errors were encountered: