Use get_data internally instead of get_data_from_viewer #2072
Conversation
rosteen
requested review from
duytnguyendtn,
javerbukh,
pllim,
kecnry,
bmorris3 and
haticekaratay
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2072 +/- ##
=======================================
Coverage ? 92.13%
=======================================
Files ? 140
Lines ? 15431
Branches ? 0
=======================================
Hits ? 14218
Misses ? 1213
Partials ? 0
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| assert list(spectral_subsets.keys()) == ['has_microns[FLUX]', | ||
| 'has_microns[FLUX] (Subset 1)', | ||
| 'has_microns[FLUX] (Subset 2)'], spectral_subsets # noqa |
There was a problem hiding this comment.
I don't remember if this paradigm for how data from get_spectra() is returned has been discussed yet but if <filename> (<subset name>) is the way we want to go, we need to update the SpecvizExample notebook cell specviz.get_spectra('Subset 1'). I am not opposed to it but I think we should run it by the team before I use the same paradigm in the get_subset work.
Co-authored-by: P. L. Lim <[email protected]>
| viewer_data = self.app.get_data_from_viewer( | ||
| self._default_spectrum_viewer_reference_name | ||
| ).get(label) | ||
| viewer_data = self.app._jdaviz_helper.get_data(data_label=label) |
There was a problem hiding this comment.
Ahh, I understand your desire to make get_data available via the app. Personally I think how it is here, while clunky, is probably a more robust way to implement it, but I could be convinced otherwise
There was a problem hiding this comment.
So is that an approval or a change request? 😅
There was a problem hiding this comment.
Haha nothing to hold out on. Just a note
|
@pllim I applied your suggestions, thanks. |
I hadn't noticed that error, but it appears to be unrelated. Looks like a unit order mismatch in Blackbody fitting. |
|
FYI I just updated the Cubeviz and Specviz example notebooks to accurately reflect the changes to |
Oh.... astropy/astropy#14439 😬 (see #2076) |
|
The output from |
|
@camipacifici I added your suggestions to the Specviz exporting data section. If the additions sound good to you I'll go ahead and merge this. |
| .. code-block:: python | ||
|
|
||
| spec = specviz.get_data(subset_to_apply='Subset 1') | ||
| subset_spec = Spectrum1D(flux=spec.flux[~spec.mask], | ||
| spectral_axis=spec.spectral_axis[~spec.mask]) | ||
| specviz.load_spectrum(subset_spec) |
There was a problem hiding this comment.
I think this makes sense for now. But since Spectrum1D already has a mask attribute, we could(/should?) make specviz.load_spectrum load the mask so that you don't have to also index by the not-mask. Does the parser do this already/is this step necessary?
There was a problem hiding this comment.
The parser does load the mask (I believe you can show the mask and uncertainties, should be a toggle somewhere), but what Cami wanted to do was load in the data such that the data outside the subset is gone completely, which is what this does.
Co-authored-by: Brett M. Morris <[email protected]>
|
Looks good to me! |
Note that I left the call in
app.get_subsets_from_viewer, since I think that will be part of @javerbukh's subset work, although I did implement something rather related inspecviz.get_spectra.I believe there is one last failing test, and I probably need to get rid of some references to the old method in the docs. Other than that I think this is ready for other eyes.