gwcs icon indicating copy to clipboard operation
gwcs copied to clipboard

Fix units in WCS API world_axis_object_components

Open Cadair opened this issue 1 year ago • 1 comments

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 avatar Sep 26 '24 14:09 Cadair

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.25%. Comparing base (eb9d316) to head (82fe891). Report is 78 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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 26 '24 14:09 codecov[bot]

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

nden avatar Oct 15 '24 23:10 nden

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.

Cadair avatar Oct 16 '24 14:10 Cadair

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

Cadair avatar Oct 21 '24 14:10 Cadair

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

nden avatar Oct 21 '24 14:10 nden

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

Cadair avatar Oct 21 '24 14:10 Cadair