Skip to content
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

Fix units in WCS API world_axis_object_components #512

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

Cadair
Copy link
Collaborator

@Cadair Cadair commented Sep 26, 2024

fixes #495

We were returning Quantity objects for CelestialFrame when we shouldn't have been, but we also need to make sure we cast the values to the units of the frame.

@Cadair Cadair requested a review from a team as a code owner September 26, 2024 14:35
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (eb9d316) to head (82fe891).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   87.28%   87.25%   -0.03%     
==========================================
  Files          22       21       -1     
  Lines        3821     3813       -8     
==========================================
- Hits         3335     3327       -8     
  Misses        486      486              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nden
Copy link
Collaborator

nden commented Oct 15, 2024

@Cadair I am trying to understand this. Can you verify that the example in the issue works now? When I run it I don't see a change with this PR. Running high_level_objects_to_values with that example does not return scalars or numpy arrays.

@Cadair
Copy link
Collaborator Author

Cadair commented Oct 16, 2024

hey @nden, here's an example:

Setup
from astropy.modeling import models
from gwcs import WCS
import gwcs.coordinate_frames as cf
import astropy.units as u
import astropy.coordinates as coord
from astropy.wcs.wcsapi.wrappers import SlicedLowLevelWCS
from astropy.wcs.wcsapi.high_level_api import high_level_objects_to_values
import numpy as np


identity = (models.Multiply(1 * u.arcsec / u.pixel) &
            models.Multiply(1 * u.arcsec / u.pixel) &
            models.Multiply(1 * u.nm / u.pixel))
sky_frame = cf.CelestialFrame(axes_order=(0, 1), name='icrs',
                              reference_frame=coord.ICRS(),
                              axes_names=("longitude", "latitude"),
                              unit=(u.arcsec, u.arcsec))
wave_frame = cf.SpectralFrame(axes_order=(2, ), unit=u.nm, axes_names=("wavelength",))

frame = cf.CompositeFrame([sky_frame, wave_frame])

detector_frame = cf.CoordinateFrame(name="detector", naxes=3,
                                    axes_order=(0, 1, 2),
                                    axes_type=("pixel", "pixel", "pixel"),
                                    axes_names=("x", "y", "z"), unit=(u.pix, u.pix, u.pix))

wcs = WCS(forward_transform=identity, output_frame=frame, input_frame=detector_frame)
values = high_level_objects_to_values(*wcs.pixel_to_world(1, 2, 3), low_level_wcs=wcs)

before:

[<Longitude 0.00027778 deg>, <Latitude 0.00055556 deg>, np.float64(3.0)]

after:

[np.float64(1.0), np.float64(2.0), np.float64(3.0)]

The issue being that before we had Quantity instances and they were not in frame units, now we have numbers in frame units.

@nden nden merged commit 9006b03 into spacetelescope:master Oct 17, 2024
21 of 22 checks passed
@nden nden added this to the 0.22 milestone Oct 17, 2024
@nden nden added the APE 14 label Oct 17, 2024
@Cadair Cadair deleted the fix_celestial_waoc branch October 17, 2024 11:00
@Cadair
Copy link
Collaborator Author

Cadair commented Oct 21, 2024

@nden are we going to be able to get this out in a release before the final release of astropy 7?

@nden
Copy link
Collaborator

nden commented Oct 21, 2024

I can get a release out but I was assumign after astropy 7.0. Or do you need this before?

@Cadair
Copy link
Collaborator Author

Cadair commented Oct 21, 2024

Well without this patch and with astropy 7 some things will break for me, so would be nice to get this out first?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in APE-14 description of WCS
2 participants