Add ability to load 2D flux arrays in Specviz #3229

rosteen · 2024-10-18T13:06:01Z

Closes #3223, opening as draft to see if this will fix the desired use case based on upstream changes. This splits a Spectrum1D with 2D flux into separate Spectrum1D objects before loading into Specviz.

…ate spectra

codecov · 2024-10-18T13:32:54Z

Codecov Report

Attention: Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.63%. Comparing base (9a1fe09) to head (f5e2f29).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/configs/specviz/plugins/parsers.py	90.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3229      +/-   ##
==========================================
+ Coverage   88.62%   88.63%   +0.01%     
==========================================
  Files         125      125              
  Lines       18775    18803      +28     
==========================================
+ Hits        16639    16666      +27     
- Misses       2136     2137       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

havok2063 · 2024-10-18T18:55:59Z

jdaviz/configs/specviz/plugins/parsers.py

+        elif path.is_file():
            try:
-                data = [Spectrum1D.read(str(path), format=format)]
-                data_label = [app.return_data_label(data_label, alt_name="specviz_data")]
+                data = Spectrum1D.read(str(path), format=format)
+                if data.flux.ndim == 2:
+                    data, data_label = split_spectrum_with_2D_flux_array(data, data_label, app)


In our file case, there are 2 data extensions, with the 1st having a 1d flux array, and the 2nd one having the nd flux array. Without the load_as_list flag, this block only loads the first extension and never reaches line 97. With load_as_list set, which is the way I think we need to load these files, I get the error

File [~/Work/jdaviz/jdaviz/configs/specviz/plugins/parsers.py:129](http://localhost:8888/lab/workspaces/auto-b/tree/~/Work/jdaviz/jdaviz/configs/specviz/plugins/parsers.py#line=128), in specviz_spectrum1d_parser(app, data, data_label, format, show_in_viewer, concat_by_file, cache, local_path, timeout, load_as_list) 127 for wlind in range(len(spec.spectral_axis)): 128 wlallorig.append(spec.spectral_axis[wlind].value) --> 129 fnuallorig.append(spec.flux[wlind].value) 131 # because some spec in the list might have uncertainties and 132 # others may not, if uncert is None, set to list of NaN. later, 133 # if the concatenated list of uncertanties is all nan (meaning 134 # they were all nan to begin with, or all None), it will be set 135 # to None on the final Spectrum1D 136 if spec.uncertainty[wlind] is not None: File [~/anaconda3/envs/newsv/lib/python3.11/site-packages/astropy/units/quantity.py:1302](http://localhost:8888/lab/workspaces/auto-b/tree/~/anaconda3/envs/newsv/lib/python3.11/site-packages/astropy/units/quantity.py#line=1301), in Quantity.__getitem__(self, key) 1297 return self._new_view( 1298 self.view(np.ndarray)[key], self.unit[key], propagate_info=False 1299 ) 1301 try: -> 1302 out = super().__getitem__(key) 1303 except IndexError: 1304 # We want zero-dimensional Quantity objects to behave like scalars, 1305 # so they should raise a TypeError rather than an IndexError. 1306 if self.isscalar: IndexError: index 1 is out of bounds for axis 0 with size 1

I think what I've done should fix this bug. It occurs when there's 2D spectra in a SpectrumList.

Yes, with your PR it loads correctly, but your PR is basically unpacking the nd arrays into individual Spectrum1D objects, which this draft code is also doing. In principle, with the change here, we don't need to manually unpack the nd fluxes inside the loader.

This draft code should work loading nd-flux data more generally, outside of the specific mwm loader

Yes, I've already reverted that back to how it was initially, see specutils#1185, ca4d8c

rileythai · 2024-10-19T02:23:46Z

Should Specviz instead just always try to load as SpectrumList first when reading single files? Given the jdaviz webapp usecase, users probably want to load all spectra in a file.

Most of the multi-extension default loaders are only implemented as SpectrumList (JWST, DESI, GALAH, SDSS), so it should try to autoload everything, right? And if it throws an IORegistryError error or can't autodetect it then it can always jump ship to the Spectrum1D case instead.

rileythai · 2024-10-19T02:39:20Z

Also on metadata -- we could make the 2D-to-1D converter split list-like metadata objects with the same length as spectrum1d.flux.shape[0] across unique metadata dicts to each split Spectrum1D, and otherwise just copy everything else.

Co-authored-by: rileythai <[email protected]>

rosteen · 2024-10-22T17:22:54Z

@havok2063 @rileythai I pulled in Riley's code from rosteen#6 handling the case of SpectrumList containing 2D spectra (and a couple other small fixes). I didn't pull in some changes I didn't agree with (some autoformatting and a couple changes to the Spectrum1D handling) but I think I got everything that was strictly necessary.

rosteen · 2024-10-22T17:31:47Z

Whoops, messed up one thing, stand by...

rosteen · 2024-10-22T18:02:28Z

Alright, I debugged the previous commit and added some more test coverage, this is ready for another look.

Codestyle

havok2063 · 2024-10-23T16:04:22Z

@rosteen @rileythai I've tested this PR with the latest version of astropy/specutils#1185 against the following file types: mwmStar, mwmVisit, spec-lite, spec-full, apStar and I was able to load all the expected extensions viz Specviz using the right format and the new load_as_list keyword as needed. Everything seems to work! Thanks to both of you for your help! I think we've converged on a working solution we're all on board with.

rosteen · 2024-10-24T13:53:03Z

@havok2063 Great, I'll mark this as ready for review then.

jdaviz/configs/specviz/plugins/parsers.py

pllim · 2024-10-24T14:30:18Z

jdaviz/configs/specviz/plugins/parsers.py

+            unc = data.uncertainty[i, :]
+        if data.mask is not None:
+            mask = data.mask[i, :]
+        new_data.append(Spectrum1D(flux=data.flux[i, :], spectral_axis=data.spectral_axis,


What about the other properties like redshift, metadata, etc? Do they not have to be propagated?

Also... is an utility function like this better suited for specutils upstream?

I just added the meta, I had discussed with Brian and we agreed that it's ok to attach the meta from the original to each split spectrum at least for now. Redshift comes along with the spectral_axis.

I think I'd prefer to keep this here for now, but we could potentially upstream it at some point. I don't know exactly where I'd want to put it in specutils.

OK, I opened astropy/specutils#1188

pllim · 2024-10-24T14:31:24Z

jdaviz/configs/specviz/tests/test_helper.py

@@ -405,6 +405,24 @@ def test_load_spectrum_list_directory_concat(tmpdir, specviz_helper):
    assert len(specviz_helper.app.data_collection) == 41


+def test_load_2d_flux(specviz_helper):
+    spec = Spectrum1D(spectral_axis=np.linspace(4000, 6000, 10)*u.Angstrom,


Maybe add comments to explain what is going on here for future devs. Hard to tell from reading the code without more context.

pllim · 2024-10-24T14:32:01Z

CHANGES.rst

@@ -15,6 +15,7 @@ Mosviz

 Specviz
 ^^^^^^^
+- Specviz parser will now split a spectrum with a 2D flux array into multiple spectra on load. [#3229]


If this is added to support a specific telescope/instrument, please state also.

Co-authored-by: P. L. Lim <[email protected]>

pllim

Since MAST is happy, I am approving by proxy and also the fact that the test added passes CI. Thanks!

javerbukh

Tested this branch and could not replicate the bug. Code looks good as well so LGTM!

rosteen · 2024-11-01T18:03:58Z

Thanks for the reviews!

Add ability to load 2D flux arrays in Specviz by splitting into separ…

d6745a5

…ate spectra

rosteen added the feature Feature request label Oct 18, 2024

rosteen added this to the 4.1 milestone Oct 18, 2024

github-actions bot added specviz plugin Label for plugins common to multiple configurations labels Oct 18, 2024

Changelog

ccc30fa

rosteen mentioned this pull request Oct 18, 2024

fix: SDSS-V SpectrumList format ambiguity, mwmVisit BOSS load fail astropy/specutils#1185

Merged

rosteen added 6 commits October 18, 2024 09:40

Handle this in file load case

9124201

Fix typo

aa0cb56

Add test and debug

180467a

Codestyle again, whoops

620c6ae

Add keyword to force Specviz parser to use SpectrumList.read

4703b9d

Add new keyword to helper load_data

fd5dc17

havok2063 reviewed Oct 18, 2024

View reviewed changes

Implement suggestions from @rileythai

cec0bea

Co-authored-by: rileythai <[email protected]>

Add more test coverage, debug

505674e

rosteen closed this Oct 22, 2024

rosteen reopened this Oct 22, 2024

rosteen added 3 commits October 22, 2024 14:04

Codestyle

2147377

Codestyle

Apparently need this to standardize the input label

47a69b1

One more label handling fix

44dcb6d

rosteen marked this pull request as ready for review October 24, 2024 13:53

rosteen requested review from javerbukh, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners October 24, 2024 13:53

pllim reviewed Oct 24, 2024

View reviewed changes

jdaviz/configs/specviz/plugins/parsers.py Show resolved Hide resolved

pllim reviewed Oct 24, 2024

View reviewed changes

jdaviz/configs/specviz/plugins/parsers.py Show resolved Hide resolved

pllim reviewed Oct 24, 2024

View reviewed changes

jdaviz/configs/specviz/plugins/parsers.py Outdated Show resolved Hide resolved

pllim reviewed Oct 24, 2024

View reviewed changes

rosteen and others added 3 commits October 24, 2024 11:40

Apply suggestions from code review

fbba848

Co-authored-by: P. L. Lim <[email protected]>

Add clarity to test and changelog

8680144

Add meta, debug

f5e2f29

pllim mentioned this pull request Oct 25, 2024

Add parser to split multi-dimensional 2D spectrum into SpectrumList of 1D spectra astropy/specutils#1188

Open

pllim approved these changes Oct 25, 2024

View reviewed changes

pllim added the Ready for final review label Oct 25, 2024

javerbukh approved these changes Nov 1, 2024

View reviewed changes

rosteen merged commit 3c14001 into spacetelescope:main Nov 1, 2024
25 checks passed

havok2063 mentioned this pull request Dec 6, 2024

Updates jdaviz to support mwmVisit files sdss/sdss_solara#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to load 2D flux arrays in Specviz #3229

Add ability to load 2D flux arrays in Specviz #3229

rosteen commented Oct 18, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

havok2063 Oct 18, 2024 •

edited

Loading

rileythai Oct 19, 2024

havok2063 Oct 21, 2024

rileythai Oct 21, 2024

rileythai commented Oct 19, 2024 •

edited

Loading

rileythai commented Oct 19, 2024 •

edited

Loading

rosteen commented Oct 22, 2024 •

edited

Loading

rosteen commented Oct 22, 2024

rosteen commented Oct 22, 2024

havok2063 commented Oct 23, 2024

rosteen commented Oct 24, 2024

pllim Oct 24, 2024

pllim Oct 24, 2024

rosteen Oct 24, 2024

pllim Oct 25, 2024

pllim Oct 24, 2024

pllim Oct 24, 2024

pllim left a comment

javerbukh left a comment

rosteen commented Nov 1, 2024

Add ability to load 2D flux arrays in Specviz #3229

Add ability to load 2D flux arrays in Specviz #3229

Conversation

rosteen commented Oct 18, 2024

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

havok2063 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileythai commented Oct 19, 2024 • edited Loading

rileythai commented Oct 19, 2024 • edited Loading

rosteen commented Oct 22, 2024 • edited Loading

rosteen commented Oct 22, 2024

rosteen commented Oct 22, 2024

havok2063 commented Oct 23, 2024

rosteen commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

javerbukh left a comment

Choose a reason for hiding this comment

rosteen commented Nov 1, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

havok2063 Oct 18, 2024 •

edited

Loading

rileythai commented Oct 19, 2024 •

edited

Loading

rileythai commented Oct 19, 2024 •

edited

Loading

rosteen commented Oct 22, 2024 •

edited

Loading