search

glue-viz / glue

Linked Data Visualizations Across Multiple Files
http://glueviz.org
Other
742 stars 153 forks source link

Fix world coordinates for 1D WCS #2345

Closed astrofrog closed 1 year ago

astrofrog commented 1 year ago

Found this while working on the tick labels in profile viewer.

dhomeier commented 1 year ago

@astrofrog some cases with result shapes like (1, N) needed an extra check to pass. broadcast_to should have outlived its existence for some time now, although since it's not strictly private, there's perhaps some small risk to break something downstream (I did test glue-jupyter and glue-astronomy against this branch).

astrofrog commented 1 year ago

@dhomeier - I had to revert https://github.com/glue-viz/glue/pull/2345/commits/a1d114c3ec11f99478189abaf162b96684cef7c7 in my latest commit - what use case required your fix?

dhomeier commented 1 year ago

This for example – result at that point was created from e.g. a length-3 array, but constructed as shape (1, 3), so the slicing result[0] was still required. https://github.com/glue-viz/glue/actions/runs/3687605543/jobs/6241424695#step:9:584 or https://github.com/glue-viz/glue/actions/runs/3903134088/jobs/6667109278#step:9:389

Maybe have to check additionally for something like result.shape[0] == 1 – what case was failing for you with the old commit?

astrofrog commented 1 year ago

Still a WIP, but getting closer

astrofrog commented 1 year ago

@dhomeier the remaining issue was that IdentityCoordinates, for single coordinates, should return a scalar or array, not a tuple with one element (this is prescribed by the APE 14 API)

dhomeier commented 1 year ago

Yes, that seemed weird, but it wasn't clear to me that came from inside glue.

dhomeier commented 1 year ago

But are you sure it is always a tuple? The self.pixel_n_dim == 1 case still does not seem to catch it.

dhomeier commented 1 year ago

a1d114c was triggered on np.ndim(result) > 1, that would mean that value is different from self.pixel_n_dim?

astrofrog commented 1 year ago

But are you sure it is always a tuple?

In principle APE 14 mandates that a tuple is returned if there are >coordinates. But there may still be cases where this doesn't happen?

The self.pixel_n_dim == 1 case still does not seem to catch it.

Is there a test failure still where this is not caught? Or can you come up with a failing example?

dhomeier commented 1 year ago

Is there a test failure still where this is not caught? Or can you come up with a failing example?

Strangely it seems to pass in most of the py310 envs, but none of the others. But on my Mac it's passing Pythons 3.9-3.11... Very weird. I thought about directly testing for a tuple, but that is failing elsewhere.

astrofrog commented 1 year ago

I think this should be fixed - it was an issue with LegacyCoordinates. Not quite sure why it would fail only on some Python versions though... Let's see if the CI is happy.

pllim commented 1 year ago

I think this broke Jdaviz, see log at https://github.com/spacetelescope/jdaviz/actions/runs/3954350017/jobs/6771598108

ValueError: input operand has more dimensions than allowed by the axis remapping

astrofrog commented 1 year ago

Investigating!

dhomeier commented 1 year ago

Seems they are all failing on glue_astronomy.spectral_coordinates.SpectralCoordinates instances of shape (1, N) which are using their own pixel_to_world_values method (even if they were subclassed on LegacyCoordinates).

dhomeier commented 1 year ago

In principle APE 14 mandates that a tuple is returned if there are >coordinates. But there may still be cases where this doesn't happen?

Does this mean it must not return a tuple for the single coordinate case? Specifically should SpectralCoordinates with n_dim=1 always return np.interp(...)[0] or tuple(np.interp(...)[0])? Either change does fix the jdaviz tests.

astrofrog commented 1 year ago

I have fixed this in a glue-astronomy PR, could you take a look at it? (Can't link to it on phone)

pllim commented 1 year ago

https://github.com/glue-viz/glue-astronomy/pull/82

pllim commented 1 year ago

glue-core needs a release. We see failure in Jdaviz now because glue-astronomy is released but not this PR.

dhomeier commented 1 year ago

These are the updates currently in

Bug Fixes

Still open

But in addition there is a new regression in