Flip output masks to match specutils formalism

[rosteen]

Glue defines a "mask" as a boolean array where True denotes valid data, whereas specutils expects the opposite (True in a mask means the data there is masked out/invalid). This PR flips the mask array in the output of the relevant data translators to match what specutils expects, fixing errors that I was getting in multiple plugins of Jdaviz due to the masks.

[codecov[bot]]

Codecov Report

Merging #17 into master will increase coverage by 0.43%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   96.64%   97.07%   +0.43%     
==========================================
  Files          13       13              
  Lines         924      924              
==========================================
+ Hits          893      897       +4     
+ Misses         31       27       -4     

Impacted Files	Coverage Δ
glue_astronomy/translators/spectral_cube.py	100.00% <ø> (ø)
glue_astronomy/translators/spectrum1d.py	96.07% <71.42%> (-3.93%)	:arrow_down:
glue_astronomy/translators/ccddata.py	100.00% <100.00%> (ø)
glue_astronomy/translators/tests/test_ccddata.py	97.67% <100.00%> (ø)
...lue_astronomy/translators/tests/test_spectrum1d.py	100.00% <100.00%> (ø)
...onomy/io/spectral_cube/tests/test_spectral_cube.py	100.00% <0.00%> (+14.28%)	:arrow_up:

---

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 ebc0737...c2c8fe6. Read the comment docs.

[rosteen]

@astrofrog FYI it was decided in offline discussions that, now that specutils more gracefully handles masks, the translators should not replace values in masked areas with NaNs. If you could take a look at this soon and merge if you approve I would be grateful, I have some features in other projects that depend on this PR.

[astrofrog]

Just to be clear, we could also merge this as-is minus the spectral-cube change, and then try and do a better job at round-tripping in a subsequent PR if this is holding anything back.

[rosteen]

Thank you both for the comments, you're right that spectralcube uses the opposite convention - I'll make that change momentarily.

I think that the issues you both raised about making sure that all the mask data round-trips correctly to glue and back through the translators is worthy of its own discussion issue and followup PR, considering that multiple related discussions have happened offline today about needing to round-trip existing masks (including more complex masks where e.g. 0 = good data and 1,2,3... = flags for various data problems) as well as uncertainty and other attributes. That will likely entail changes both here and in jdaviz, but I'll make the discussion issue here.

[rosteen]

Just pushed an update after realizing that it was not handling collapsing the mask properly to 1d in the case of cube data. Now it should return a mask of the correct shape that is all False for cases where there was spatial subsetting but not spectral, and a mask with True in the correct locations for a case of a cube subsetted on the spectral axis. It assumes that the Glue data object always uses axis 0 for the spectral axis - if that isn't the case, this will need to be smarter.

I also uncovered another problem that is not solved here (I think it would require a Glue change): part of the changes here were to stop replacing masked values with NaN, but the data.compute_statistic function seems to still do that while collapsing >1D data down to 1D.

[eteq]

Hmm. While the CI seems to be failing for the wrong reasons, when I run this locally I see:

FAILED glue_astronomy/translators/tests/test_ccddata.py::test_to_ccddata[False] - AssertionError:
FAILED glue_astronomy/translators/tests/test_ccddata.py::test_to_ccddata[True] - AssertionError:
FAILED glue_astronomy/translators/tests/test_spectrum1d.py::test_to_spectrum1d - AssertionError:
=========================== 3 failed, 50 passed, 2 skipped, 1 xfailed, 30 warnings in 1.87s ============================

are you seeing the same, @rosteen ?

[eteq]

The remaining CI failures look like red herrings to me and things are now passing locally, so I think we're all good. Thanks @rosteen !