BUG: Fix inaccurate coordinates info on dithered data linked by WCS #992
Conversation
and update tests and notebook. Also add in backend to show which data is currently visible.
pllim
force-pushed
the
the-coords-who-cried-wolf
branch
from
5dc43f1 to
f68eea6
Compare
pllim
requested review from
duytnguyendtn,
ibusko,
javerbukh,
kecnry,
ojustino and
rosteen
as code owners
Codecov Report
@@ Coverage Diff @@
## main #992 +/- ##
==========================================
+ Coverage 70.53% 71.55% +1.02%
==========================================
Files 74 74
Lines 5437 5464 +27
==========================================
+ Hits 3835 3910 +75
+ Misses 1602 1554 -48
Continue to review full report at Codecov.
|
| { | ||
| "cell_type": "markdown", | ||
| "id": "81249347-a61b-478e-bcfc-586f0457ff4e", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "This next cell is only enable for debugging. It captures outputs printed to the underlying application `Output` widget." | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "raw", | ||
| "id": "d23931dc", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "# This is for debugging only.\n", | ||
| "imviz.app._application_handler.output" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Do we want to keep this in the public example notebook?
There was a problem hiding this comment.
Maybe until we can find a way to display which data is currently visible. Every time we have Imviz "hack hour" or something, people ask about that feature. At least for now, we can ask them to enable this cell and see the print out.
| link_type = 'pixels' | ||
| else: # If not pixels, must be WCS | ||
| link_type = 'wcs' | ||
| break # Might have duplicate, just grab first match |
There was a problem hiding this comment.
In what scenario would this happen (and would the "duplicate" ever be something different than the first match)?
There was a problem hiding this comment.
In this scenario:
>>> import numpy as np
>>> from jdaviz import Imviz
>>> a = np.ones((10, 10))
>>> imviz = Imviz()
>>> imviz.load_data(a, data_label='1')
>>> imviz.load_data(a, data_label='2')
>>> imviz.load_data(a, data_label='3')
>>> for elink in imviz.app.data_collection.external_links:
... print(elink.__class__.__name__, elink.data1.label, elink.data2.label)
LinkSame 1 2
LinkSame 1 2
LinkSame 1 3
LinkSame 1 3Figuring out why is a rabbit hole for another day.
There was a problem hiding this comment.
So far in my limited testing, the duplicate is always the same.
There is also another oddity I encountered -- When I load pure numpy arrays, it takes one extra blink for the app to report the correct visible layer data label. Again, "future work."
There was a problem hiding this comment.
ok, as long as we would never expect the scenario where the first case gives LinkSame -> pixels but another (ignored because of the break) case would give wcs, then I think this is probably fine.
Description
This pull request is to fix inaccurate coordinates info on dithered data linked by WCS.
Also add in backend to show which data is currently visible. Adding this info to GUI is a separate ticket and directly tied to Imviz wireframe work.
Suggested workflow for review:
bto blink. Currently, they are linked by pixels, so X/Y should stay the same but WCS and value will change.bto blink. Now, they are linked by WCS, so X/Y and value will change, but WCS still stay the same.If you also run with the JWST data, please note that there is upstream bug reported at spacetelescope/gwcs#392.
Also feel free to load your favorite data to test this out. Everything is linked by pixels by default (on load), so you always need to run the extra cell to link them by WCS.
Fixes #945
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.CHANGES.rst?