nmearl commented 5 years ago

Requires spacetelescope/specviz#671.

This resolves #386, and resolves #364, and resolves #330. It also resolves #477.

astrofrog commented 5 years ago

I just tried this out and ran into the following:

With the manga-7495-12704-LOGCUBE.fits test cube I get:

'1E-17 erg/s/cm^2/Ang/spaxel' did not parse as unit: At col 17, Ang is not a valid unit. Did you mean aG, aN, ag, nG or ng?

By default it looks like specviz is occupying too much space:

I'm using a 13" screen. I can't actually shrink specviz without making it disappear completely (i.e. the minimum size is too large)

There is no longer an indicator of the current slice in specviz
If I click on the vertical line for zero in specviz and drag, a new region is created

I was also asked by @javerbukh to check the status of a few issues:

https://github.com/spacetelescope/cubeviz/issues/386 (Specviz no longer responds to flux units changes) is still a problem
https://github.com/spacetelescope/cubeviz/issues/364 (Check cubeviz for listener on units change from specviz) is still a problem
I can't test https://github.com/spacetelescope/cubeviz/issues/330 because there is no longer an indicator of the current slice in specviz
https://github.com/spacetelescope/cubeviz/issues/301 is more of a design decision about behavior, so not really something we should worry about here if we care about feature parity.

nmearl commented 5 years ago

Thanks @astrofrog!

* If I click on the vertical line for zero in specviz and drag, a new region is created

Can you elaborate on this a bit, I'm not sure I follow?

* I can't test #330 because there is no longer an indicator of the current slice in specviz

The latest commits should include the slice indicator.

javerbukh commented 5 years ago

Specviz seems to work as expected with the following files: DATACUBE_HDFS_v1p0.fits.gz, FLAMES_Haro11_L6_cube_bjames.fits, GMOS_NGC4449_red_bjamesv2.fits, SINFONI_CAS64_k_bjames.fits, KLASS_KMOS_COMBINE_SCI_RECONSTRUCTED_S2A_1261.fits
The following file types, data did not appear in the Specviz window because of an error: JWST-NIRSpec-IFU-GAL2136_CLEAR_PRISM.fits(error: "Failed to add data 'COUNTS METER-1 SLICERPOS-2' did not parse as unit: At col 0, COUNTS is not a valid unit. Did you mean count?"), manga-7495-12704-LOGCUBE.fits (error: same as Tom's), and bx442_OSIRIS.fits (error: "Data is not a 1D spectrum").
The UI is looking good and I am not getting the same spacing issue that Tom has in his screenshot (I am on a 15 inch macbook).
The slice indicator is working as expected.
I was not able to replicate what Tom was mentioning about creating a new region from clicking the vertical line above 0.

hcferguson commented 5 years ago

I tried jw10001001001_01101_00001_mirifushort_s3d_ref.fits in Nick's master cubeviz. It gives a bunch of warnings in the popup box:

Checking filename /Users/hcferguson/datb_data/cubeviz_190222/jw10001001001_01101_00001_mirifushort_s3d_ref.fits

HDU /Users/hcferguson/datb_data/cubeviz_190222/jw10001001001_01101_00001_mirifushort_s3d_ref.fits_0
    CTYPE1 is NONE, setting to RA---TAN
    CTYPE2 is NONE, setting to DEC--TAN
    CTYPE3 is NONE, setting to WAVE-TAB
    CUNIT1 is NONE, setting to deg
    CUNIT2 is NONE, setting to deg
    CUNIT3 is NONE, setting to um
    Setting HDU 0 EXTNAME field to /Users/hcferguson/datb_data/cubeviz_190222/jw10001001001_01101_00001_mirifushort_s3d_ref.fits_0

HDU DQ
    CTYPE1 is NONE, setting to RA---TAN
    CTYPE2 is NONE, setting to DEC--TAN
    CTYPE3 is NONE, setting to WAVE-TAB
    CUNIT1 is NONE, setting to deg
    CUNIT2 is NONE, setting to deg
    CUNIT3 is NONE, setting to um

HDU ERR
    CTYPE1 is NONE, setting to RA---TAN
    CTYPE2 is NONE, setting to DEC--TAN
    CTYPE3 is NONE, setting to WAVE-TAB
    CUNIT1 is NONE, setting to deg
    CUNIT2 is NONE, setting to deg
    CUNIT3 is NONE, setting to um

HDU WMAP
    CTYPE1 is NONE, setting to RA---TAN
    CTYPE2 is NONE, setting to DEC--TAN
    CTYPE3 is NONE, setting to WAVE-TAB
    CUNIT1 is NONE, setting to deg
    CUNIT2 is NONE, setting to deg
    CUNIT3 is NONE, setting to um

If I click "accept" I see some data, but the image windows are inactive (can't get the mouse focus away from the specviz window in order to use the menus; but the cursor does show values).

Traceback on the terminal is:

(cubeviz-master) ~/d/cubeviz_190222 ❯❯❯ cubeviz
objc[2251]: Class FIFinderSyncExtensionHost is implemented in both /System/Library/PrivateFrameworks/FinderKit.framework/Versions/A/FinderKit (0x7fff966d6cd0) and /System/Library/PrivateFrameworks/FileProvider.framework/OverrideBundles/FinderSyncCollaborationFileProviderOverride.bundle/Contents/MacOS/FinderSyncCollaborationFileProviderOverride (0x13c53acd8). One of the two will be used. Which one is undefined.
WARNING:ifcube: HDU 0 has no EXTNAME field
WARNING:glue.core.coordinates:

*******************************
Encounted an error during WCS parsing. Discarding world coordinates! 
ERROR 5 in wcsset() at line 2176 of file cextern/wcslib/C/wcs.c:
Invalid parameter value.
ERROR 3 in tabset() at line 749 of file cextern/wcslib/C/tab.c:
Invalid tabular parameters: Each element of K must be positive, got 0.

*******************************

/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/core/data.py:1637: UserWarning: Data.visible_components is deprecated
  warnings.warn('Data.visible_components is deprecated', UserWarning)
Traceback (most recent call last):
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/app/qt/application.py", line 236, in _choose_load_data_wizard
    self._choose_load_data(data_importer=data_wizard)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/app/qt/application.py", line 255, in _choose_load_data
    app.add_datasets(app.data_collection, data)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/core/application_base.py", line 302, in add_datasets
    data_collection.extend(datasets)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/core/data_collection.py", line 98, in extend
    self.append(d)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/core/data_collection.py", line 84, in append
    self.hub.broadcast(msg)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/glue/core/hub.py", line 217, in broadcast
    handler(message)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/cubeviz/listener.py", line 51, in handle_new_dataset
    self.configure_layout(data)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/cubeviz/listener.py", line 61, in configure_layout
    self.setup_data(cubeviz_layout, data)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/cubeviz/listener.py", line 120, in setup_data
    cubeviz_layout.add_data(data)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/cubeviz/layout.py", line 637, in add_data
    self._flux_unit_controller.set_data(data)
  File "/Users/hcferguson/anaconda3/envs/cubeviz-master/lib/python3.6/site-packages/cubeviz/controls/flux_units.py", line 326, in set_data
    wcs = data.coords.wcs
AttributeError: 'Coordinates' object has no attribute 'wcs'

hcferguson commented 5 years ago

Hmm...possible clue: Even though I selected jwst-fits-cube, it's showing the filename as manga: jw10001001001_01101_00001_mirifushort_s3d_ref.

If I go down one in the menu and select jwst-asdf-cube it gives me the same warnings but labels it jwst-fits-cube: jw10001001001_01101_00001_mirifushort_s3d_ref. So menu seems to have an indexing problem.

drdavella commented 5 years ago

@nmearl it looks like this needs to be rebased. Also there are some real failures on CI that need to be addressed.

nmearl commented 5 years ago

Indeed. Most of the specific failures will fall out once the specviz PR is merged.

nmearl commented 5 years ago

@drdavella It seems that the tests are failing because of fundamental incompatibilies between glue 0.13 and glue 0.14+. I'm not sure we can refactor the specviz data viewer to be compatible with both. I'm waiting on a response to @astrofrog to see if the schism between the versions is too great, or if it really is maybe just some easy refactoring (my preliminary look over the code base suggests it's not, but Tom might have some guru-level code bridge that could Solve All The Problems™️).

How amenable are we to dropping support for 0.13 if using the latest specviz?

drdavella commented 5 years ago

@nmearl I'm perfectly amenable to dropping support for 0.13. We have no compelling reason to support more than one version right now.

hcferguson commented 5 years ago

@nmearl From my perspective, it's critical to have all of the viz's in sync on which version of glue they require. Actually, it's even important to extend this across all the tools, including ginga, so that we can have one Astroconda distribution rather than several. If some big change to glue comes along to force us to refactor code, we should try to sync that up.

I'm not sure what the implications of this general philosophy are for this particular case.

Looking at the Astroconda packages, it looks like it's several versions of glue back. So this might require consultation with @pllim about ginga/glue dependency and @jhunkeler about Astroconda.

pllim commented 5 years ago

Ginga has no dependency on Glue and given that I am not involved in the Viz stuff, I have no strong opinion. Did you consult Tom R on this? You can go the route of pipeline and define a specific pinned environment YAML for your users. :woman_shrugging:

The last time we did a max version pinning at astroconda, I think it ended in disaster. I wouldn't advise it unless you have no other choice.

nmearl commented 5 years ago

I cannot reproduce the error in test_ui locally -- @drdavella could you try running the test suite locally and let me know if you're seeing it?

SaOgaz commented 5 years ago

I spoke to @jhunkeler offline (well, off github) about the astroconda glue issue, and he made a reasonable case for us not being able to support glue in astroconda. I think we can still keep our tool releases using consistent versions of glue, while having users pull glue from other sources (from the glue channel or from conda-forge).

drdavella commented 5 years ago

@nmearl I'm not seeing the failure locally either. Will try to investigate more in a bit.

drdavella commented 5 years ago

@nmearl can you try setting the pytest fixture scope back to "session"? The tests seem to work fine for me locally when using session scope, and I'm wondering if it will resolve the error we see currently.

drdavella commented 5 years ago

See #498 which reverted the fixture scope from "module" back to "fixture". This appeared to make the tests pass.

codecov[bot] commented 5 years ago

Codecov Report

Merging #484 into master will increase coverage by 1.37%. The diff coverage is 76.31%.

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage   62.31%   63.69%   +1.37%     
==========================================
  Files          40       39       -1     
  Lines        5069     4942     -127     
==========================================
- Hits         3159     3148      -11     
+ Misses       1910     1794     -116

Impacted Files	Coverage Δ
cubeviz/controls/wavelengths.py	`96.82% <ø> (-1.69%)`	:arrow_down:
cubeviz/controls/slice.py	`93.15% <100%> (+2.67%)`	:arrow_up:
cubeviz/controls/tests/test_slice_controller.py	`100% <100%> (ø)`	:arrow_up:
cubeviz/image_viewer.py	`59.12% <100%> (+5.19%)`	:arrow_up:
cubeviz/tools/collapse_cube.py	`69.67% <33.33%> (-0.17%)`	:arrow_down:
cubeviz/layout.py	`82.89% <72%> (-0.01%)`	:arrow_down:
cubeviz/keyboard_shortcuts.py	`44.23% <0%> (-1.93%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update edaa1b3...ee54142. Read the comment docs.

drdavella commented 5 years ago

After testing this locally I noticed that ROI deletion in cube viewers does not propagate properly to the spectrum viewer. The corresponding subset appears to be removed from the list of data items in specviz, but the deletion is not reflected in the plot itself.

All other ROI updates appear to propagate correctly, though.

nmearl commented 5 years ago

@drdavella Fixed in spacetelescope/specviz#680.

nmearl commented 5 years ago

spacetelescope/specviz#680 has been merged.

nmearl commented 5 years ago

@drdavella @brechmos-stsci Any chance of a re-review?

spacetelescope / cubeviz

Integration of latest SpecViz #484

Codecov Report