Add workaround for Spectrum1D GWCS links #1526
Conversation
rosteen
requested review from
duytnguyendtn,
javerbukh,
ojustino,
pllim and
kecnry
as code owners
…self Add changelog entry
rosteen
force-pushed
the
fix-specviz-linking
branch
from
f5070cf to
b1c0825
Compare
There was a problem hiding this comment.
Confirmed this fixes the case I had to reproduce the original bug. It would be nice to see this fixed upstream, but since this used to work in jdaviz, I vote we get this workaround out as a bugfix in the meantime.
I'm happy to approve once understanding if the raise is intentional (I think we might need it gone for Gaussian Smooth to work correctly). Edit: and hopefully that is the reason CI is very upset and that will pass soon 🤞
|
Still has a test failure, I'll look into it tomorrow. Might just need to update the expected test result. |
|
The remaining test failure was an unrelated SSL timeout. |
Codecov Report
@@ Coverage Diff @@
## main #1526 +/- ##
=======================================
Coverage 85.37% 85.37%
=======================================
Files 91 91
Lines 8624 8631 +7
=======================================
+ Hits 7363 7369 +6
- Misses 1261 1262 +1
Continue to review full report at Codecov.
|
| if len(self.data_collection) > 1: | ||
| self._link_new_data() |
There was a problem hiding this comment.
just trying to wrap my head around the need for all the changes in tests - is avoiding self-linking (alone) responsible for dropping all those links and affecting the indices?
There was a problem hiding this comment.
I believe so, yes. In Cubeviz there were 3 extra links, Specviz 1 extra. Hence the changes.
Where is the upstream issue? Thanks! |
Fixes #1525, although this should be considered a temporary workaround until (hopefully) upstream fixes happen. The root cause of the issue is that Glue's
WCSLinklooks for ahas_celestialattribute on thedata.coordsobject. In the case of aSpectrum1Dthat's initialized with a spectral_axis array rather than a WCS (and thus creates a lookup table GWCS), glue_astronomy creates a specialSpectralCoordinatesobject for thedata.coordsthat was missing thehas_celestialattribute. This caused anAttributeErrorevery time and thus the linking to fall back on pixel links.Unfortunately, after I locally added
has_celestial=Falseto theSpectralCoordinates, theWCSLinkseemed to still fail. So here we fall back on the old explicit manual linking for this case while more debugging happens upstream.Note that I also took the opportunity here to stop linking the first data to itself, since (I'm pretty sure) it's unnecessary.