Spectrum1D translator update for NDCube version #36

rosteen · 2021-07-09T17:32:01Z

This updates the Spectrum1D translator to handle 3D Spectrum1D objects, currently applicable to the dev version of specutils which subclasses Spectrum1D off of NDCube. I also tested with the current releases of specutils and jdaviz and this doesn't break back-compatibility, so it should be fine to merge it now.

This also supersedes #29 .

codecov · 2021-07-09T17:34:26Z

Codecov Report

Merging #36 (7e2cc09) into master (4c5e6e7) will decrease coverage by 1.62%.
The diff coverage is 68.51%.

❗ Current head 7e2cc09 differs from pull request most recent head 7a01dbf. Consider uploading reports for the commit 7a01dbf to get more accurate results

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   96.40%   94.77%   -1.63%     
==========================================
  Files          15       15              
  Lines        1002     1034      +32     
==========================================
+ Hits          966      980      +14     
- Misses         36       54      +18

Impacted Files	Coverage Δ
...lue_astronomy/translators/tests/test_spectrum1d.py	`89.43% <63.63%> (-10.57%)`	⬇️
glue_astronomy/translators/spectrum1d.py	`91.66% <76.19%> (-5.56%)`	⬇️

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 4c5e6e7...7a01dbf. Read the comment docs.

rosteen · 2021-07-09T17:46:03Z

@javerbukh it would be great if you could test this out with your jdaviz branch for spacetelescope/jdaviz#547. Actually, now that I think about it, I think I made a few minor changes to your PR - I'll open a PR to your fork with those.

Edit: see javerbukh/jdaviz#7

javerbukh · 2021-07-12T18:40:24Z

Are you able to load data with this branch and jdaviz#547 + your additions?

rosteen · 2021-07-12T18:45:47Z

I am. You also need the dev version of specutils (which in turn installs a dev version of ndcube). Order of installation also might matter, make sure you install the jdaviz code first to make sure it doesn't try to upgrade the version of glue-jupyter or specutils.

javerbukh · 2021-07-12T18:51:17Z

Ah, gotcha.

javerbukh · 2021-07-12T18:54:58Z

Ok, data loads! It looks off but was the priority that it loaded?

rosteen · 2021-07-12T19:02:06Z

Excellent. What about the data looks off? The only problem I noticed initially was that the slider is broken (which isn't a glue-astronomy problem).

If you could also check that the data round-trips correctly by pulling it back out into the notebook, that would probably be sufficient for this PR.

javerbukh · 2021-07-12T19:15:09Z

Looks like RA and Dec are flipped: https://jdaviz.readthedocs.io/en/latest/cubeviz/displaycubes.html#defining-spatial-regions

rosteen · 2021-07-12T19:16:43Z

Ah, right, they probably are. Could you install specutils from astropy/specutils#822 and see if that fixes the RA/Dec swap?

Lots of dev versions/PRs to get this all working nicely at the moment, unfortunately.

javerbukh · 2021-07-12T19:22:30Z

That works! I'll have to update the jdaviz PR to have the slider use the correct axis.

rosteen · 2021-07-16T12:48:37Z

@astrofrog @eteq I can't tag reviewers here, would one of you take a look at this and approve/merge if it looks good to you (or comment if it doesn't)? I believe the non-codecov test failures are due to failed tox invocations, not something due to this PR.

Of course now that I mention codecov...I should add a test of round tripping a 3d Spectrum1D. I'll see if I have time for that today.

rosteen · 2021-07-22T15:29:22Z

Codecov is angry because the added tests don't run right now, since they require NDCube 2.0.

pllim · 2021-08-17T20:10:52Z

Can we merge?

astrofrog

Looks good in general, just a couple of requests

astrofrog · 2021-08-24T23:24:24Z

glue_astronomy/translators/spectrum1d.py

+        # Glue expects spectral axis first for cubes (opposite of specutils).
+        # Swap the spectral axis to first here. to_object doesn't need this because
+        # Spectrum1D does it automatically on initialization.
+        if len(obj.flux.shape) == 3:


What about cases where the data has 2 dimensions? (for example a 2D spectrum)

astrofrog · 2021-08-24T23:24:49Z

CHANGES.rst

+0.1 (2020-09-17)
+----------------
+
+- Initial release


Could you rebase so as not to include this commit? You'll then need to add a 0.3 section and a changelog entry.

astrofrog · 2021-08-24T23:25:17Z

glue_astronomy/translators/spectrum1d.py

+        else:
+            # Don't use the dummy GWCS created by Spectrum1D initialized with spectral_axis
+            if isinstance(obj.wcs, GWCS):
+                data = Data(coords=SpectralCoordinates(obj.spectral_axis))


We don't really need this anymore, Data coords can be a GWCS as far as I know.

astrofrog · 2021-08-24T23:43:32Z

glue_astronomy/translators/spectrum1d.py

-        data['flux'] = obj.flux
-        data.get_component('flux').units = str(obj.unit)
+
+        # Glue expects spectral axis first for cubes (opposite of specutils).


In addition to the above comments it would be good to clarify here whether you mean for the WCS or Numpy array. The data array and WCS will have axes in opposite order - spectral cubes read with plain Astropy FITS and WCS or with SpectralCube are often stored in spectral,dec,ra for the Numpy data array and ra,dec,spectral for the WCS.

Glue can handle any axis order as long as x_att and y_att are set correctly but it would indeed be good to make sure SpectralCube and Spectrum1D return a similar glue data object.

Fix to use SpectralCoordinate

Working on better 3D Spectrum1D tests

Finish setting up new Spectrum1D tests Fix codestyle errors Allow test to run on 2.0.dev

astrofrog · 2021-08-27T12:51:28Z

I have rebased this and will merge and address my comments in a follow-up PR (I need to make further changes to this to support 2D Spectrum1D instances.

astrofrog · 2021-08-27T19:42:21Z

The test failure with dev dependencies is related as I think it's a recent change in specutils but I will merge and address in a separate PR.

pllim · 2021-08-30T14:35:27Z

@astrofrog , I think this causing failure in jdaviz dev job, see spacetelescope/jdaviz#821 .

rosteen mentioned this pull request Jul 9, 2021

Updates to match glue-astronomy PR javerbukh/jdaviz#7

Merged

javerbukh mentioned this pull request Jul 12, 2021

SpectralCube to specutils transition in cubeviz spacetelescope/jdaviz#547

Merged

3 tasks

rosteen force-pushed the spec1d-translator-update branch from 0278e6e to 7e2cc09 Compare July 22, 2021 15:24

rosteen force-pushed the spec1d-translator-update branch from 7e2cc09 to 48a4fbe Compare July 22, 2021 15:46

pllim mentioned this pull request Aug 17, 2021

BUG: Do not crash when masking fails #39

Closed

astrofrog requested changes Aug 24, 2021

View reviewed changes

astrofrog reviewed Aug 24, 2021

View reviewed changes

astrofrog-conda-forge mentioned this pull request Aug 25, 2021

WCS and class for 2d spectra in MOSViz spacetelescope/jdaviz#804

Closed

rosteen added 5 commits August 27, 2021 13:50

Update Spectrum1D translator to work with NDCube-based 3D cubes

010b9ab

Handling 1D dummy GWCS case

2940e3d

Fix to use SpectralCoordinate

Working on better 3D Spectrum1D tests

1002ca8

Working on better 3D Spectrum1D tests

Fix problems with uncertainty/mask axis swapping and collapsed coords

cdd844d

Working on new getting new unit test to pass

7a01dbf

Finish setting up new Spectrum1D tests Fix codestyle errors Allow test to run on 2.0.dev

astrofrog force-pushed the spec1d-translator-update branch from 48a4fbe to 7a01dbf Compare August 27, 2021 12:51

astrofrog merged commit 03e439a into glue-viz:master Aug 27, 2021

astrofrog mentioned this pull request Aug 27, 2021

Use different coords for 3d and 1d Spectrum1D objects #29

Closed

This was referenced Aug 30, 2021

BUG: Gaussian smooth plugin incompatible with unreleased upstream package spacetelescope/jdaviz#821

Closed

Change Spectrum1DHandler default stat back to mean #40

Merged

Change Spectrum1DHandler default back to statistic=None #43

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spectrum1D translator update for NDCube version #36

Spectrum1D translator update for NDCube version #36

rosteen commented Jul 9, 2021

codecov bot commented Jul 9, 2021 •

edited

Loading

rosteen commented Jul 9, 2021 •

edited

Loading

javerbukh commented Jul 12, 2021

rosteen commented Jul 12, 2021

javerbukh commented Jul 12, 2021

javerbukh commented Jul 12, 2021

rosteen commented Jul 12, 2021 •

edited

Loading

javerbukh commented Jul 12, 2021 •

edited

Loading

rosteen commented Jul 12, 2021

javerbukh commented Jul 12, 2021

rosteen commented Jul 16, 2021

rosteen commented Jul 22, 2021

pllim commented Aug 17, 2021

astrofrog left a comment

astrofrog Aug 24, 2021

astrofrog Aug 24, 2021

astrofrog Aug 24, 2021

astrofrog Aug 24, 2021

astrofrog commented Aug 27, 2021

astrofrog commented Aug 27, 2021

pllim commented Aug 30, 2021

Spectrum1D translator update for NDCube version #36

Spectrum1D translator update for NDCube version #36

Conversation

rosteen commented Jul 9, 2021

codecov bot commented Jul 9, 2021 • edited Loading

Codecov Report

rosteen commented Jul 9, 2021 • edited Loading

javerbukh commented Jul 12, 2021

rosteen commented Jul 12, 2021

javerbukh commented Jul 12, 2021

javerbukh commented Jul 12, 2021

rosteen commented Jul 12, 2021 • edited Loading

javerbukh commented Jul 12, 2021 • edited Loading

rosteen commented Jul 12, 2021

javerbukh commented Jul 12, 2021

rosteen commented Jul 16, 2021

rosteen commented Jul 22, 2021

pllim commented Aug 17, 2021

astrofrog left a comment

Choose a reason for hiding this comment

astrofrog Aug 24, 2021

Choose a reason for hiding this comment

astrofrog Aug 24, 2021

Choose a reason for hiding this comment

astrofrog Aug 24, 2021

Choose a reason for hiding this comment

astrofrog Aug 24, 2021

Choose a reason for hiding this comment

astrofrog commented Aug 27, 2021

astrofrog commented Aug 27, 2021

pllim commented Aug 30, 2021

codecov bot commented Jul 9, 2021 •

edited

Loading

rosteen commented Jul 9, 2021 •

edited

Loading

rosteen commented Jul 12, 2021 •

edited

Loading

javerbukh commented Jul 12, 2021 •

edited

Loading