Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use get_data internally instead of get_data_from_viewer #2072

Merged
merged 16 commits into from
Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ API Changes

- Export plot plugin now exposes the ``viewer`` dropdown in the user API. [#2037]

- Replaced internal ``get_data_from_viewer()`` calls, ``specviz.get_spectra`` now returns
spectra for all data+subset combinations. [#2072]

Cubeviz
^^^^^^^

Expand Down
4 changes: 3 additions & 1 deletion jdaviz/configs/cubeviz/plugins/tests/test_regions.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ def test_spatial_spectral_mix(self):
# https://github.com/spacetelescope/jdaviz/issues/1584
with pytest.warns(UserWarning, match='Applying the value from the redshift slider'):
spectral_subsets = self.cubeviz.specviz.get_spectra()
assert list(spectral_subsets.keys()) == ['has_microns[FLUX]', 'Subset 1', 'Subset 2'], spectral_subsets # noqa
assert list(spectral_subsets.keys()) == ['has_microns[FLUX]',
'has_microns[FLUX] (Subset 1)',
'has_microns[FLUX] (Subset 2)'], spectral_subsets # noqa
Comment on lines +72 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for sp in spectral_subsets.values():
assert isinstance(sp, Spectrum1D)
4 changes: 1 addition & 3 deletions jdaviz/configs/default/plugins/line_lists/line_lists.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,7 @@ def _on_viewer_data_changed(self, msg=None):

label = msg.data.label
try:
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is that an approval or a change request? 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha nothing to hold out on. Just a note

except TypeError:
warn_message = SnackbarMessage("Line list plugin could not retrieve data from viewer",
sender=self, color="error")
Expand Down
25 changes: 15 additions & 10 deletions jdaviz/configs/default/plugins/model_fitting/model_fitting.py
Original file line number Diff line number Diff line change
Expand Up @@ -294,15 +294,20 @@ def _warn_if_no_equation(self):

def _get_1d_spectrum(self):
# retrieves the 1d spectrum (accounting for spatial subset for cubeviz, if necessary)
if self.config == 'cubeviz' and self.spatial_subset_selected != 'Entire Cube':
# then we're acting on the auto-collapsed data in the spectrum-viewer
# of a spatial subset. In the future, we may want to expose on-the-fly
# collapse options... but right now these will follow the settings of the
# spectrum-viewer itself
return self.app.get_data_from_viewer(
self._default_spectrum_viewer_reference_name,
self.spatial_subset_selected
)
if self.config == 'cubeviz':
stat = self.app.get_viewer(self._default_spectrum_viewer_reference_name).state.function
if self.spatial_subset_selected == 'Entire Cube':
return self.app._jdaviz_helper.get_data(data_label=self.dataset_selected,
statistic=stat)
else:
# then we're acting on the auto-collapsed data in the spectrum-viewer
# of a spatial subset. In the future, we may want to expose on-the-fly
# collapse options... but right now these will follow the settings of the
# spectrum-viewer itself
kwargs = {"data_label": self.dataset_selected,
"subset_to_apply": self.spatial_subset_selected,
"statistic": stat}
return self.app._jdaviz_helper.get_data(**kwargs)
else:
return self.dataset.selected_obj

Expand All @@ -321,7 +326,7 @@ def _dataset_selected_changed(self, event=None):
IPyWidget callback event object. In this case, represents the data
label of the data collection object selected by the user.
"""
if not hasattr(self, 'dataset'):
if not hasattr(self, 'dataset') or self.app._jdaviz_helper is None:
# during initial init, this can trigger before the component is initialized
return

Expand Down
13 changes: 10 additions & 3 deletions jdaviz/configs/imviz/plugins/coords_info/coords_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,14 @@ def _spectrum_viewer_update(self, viewer, x, y):
if not isinstance(lyr.layer.subset_state, RoiSubsetState):
# then this is a SPECTRAL subset
continue
elif ((not isinstance(lyr.layer, BaseData)) or (lyr.layer.ndim not in (1, 3))
or (not lyr.visible)):
# For use later in data retrieval
subset_label = lyr.layer.label
data_label = lyr.layer.data.label
elif ((not isinstance(lyr.layer, BaseData)) or (lyr.layer.ndim not in (1, 3))):
continue
else:
subset_label = None
data_label = lyr.layer.label

try:
# Cache should have been populated when spectrum was first plotted.
Expand All @@ -339,7 +344,9 @@ def _spectrum_viewer_update(self, viewer, x, y):
if cache_key in self.app._get_object_cache:
sp = self.app._get_object_cache[cache_key]
else:
sp = self.app.get_data_from_viewer('spectrum-viewer', lyr.layer.label)
sp = self.app._jdaviz_helper.get_data(data_label=data_label,
statistic=statistic,
subset_to_apply=subset_label)
self.app._get_object_cache[cache_key] = sp

# Out of range in spectral axis.
Expand Down
43 changes: 37 additions & 6 deletions jdaviz/configs/specviz/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from astropy import units as u
from astropy.utils.decorators import deprecated
from glue.core.subset_group import GroupedSubset
from specutils import SpectralRegion, Spectrum1D

from jdaviz.core.helpers import ConfigHelper
Expand Down Expand Up @@ -67,20 +68,50 @@ def load_spectrum(self, data, data_label=None, format=None, show_in_viewer=True,
show_in_viewer=show_in_viewer,
concat_by_file=concat_by_file)

def get_spectra(self, data_label=None, apply_slider_redshift="Warn"):
def get_spectra(self, data_label=None, subset_to_apply=None, apply_slider_redshift="Warn"):
"""Returns the current data loaded into the main viewer

"""
spectra = self.app.get_data_from_viewer(
self._default_spectrum_viewer_reference_name, data_label=data_label
)
spectra = {}
# Just to save line length
get_data_method = self.app._jdaviz_helper.get_data
viewer = self.app.get_viewer(self._default_spectrum_viewer_reference_name)
statistic = getattr(viewer, "function", None)

if data_label is not None:
spectrum = get_data_method(data_label=data_label,
subset_to_apply=subset_to_apply,
statistic=statistic)
spectra[data_label] = spectrum
else:
for layer_state in viewer.state.layers:
lyr = layer_state.layer
if subset_to_apply is not None:
if lyr.label == subset_to_apply:
spectrum = get_data_method(data_label=lyr.data.label,
subset_to_apply=subset_to_apply,
statistic=statistic)
spectra[lyr.data.label] = spectrum
else:
continue
else:
if isinstance(lyr, GroupedSubset):
spectrum = get_data_method(data_label=lyr.data.label,
subset_to_apply=lyr.label,
statistic=statistic)
spectra[f'{lyr.data.label} ({lyr.label})'] = spectrum
else:
spectrum = get_data_method(data_label=lyr.label,
statistic=statistic)
spectra[lyr.label] = spectrum

if not apply_slider_redshift:
if data_label is not None:
return spectra[data_label]
return spectra
else:
output_spectra = {}
# We need to create new Spectrum1D outputs with the redshifts set
if data_label is not None:
spectra = {data_label: spectra}
for key in spectra.keys():
output_spectra[key] = _apply_redshift_to_spectra(spectra[key], self._redshift)

Expand Down
44 changes: 26 additions & 18 deletions jdaviz/configs/specviz/plugins/line_analysis/line_analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,10 @@ def _on_viewer_data_changed(self, msg):
if msg is None or msg.viewer_id != viewer_id or msg.data is None:
return

viewer_data = self.app.get_data_from_viewer(
self._default_spectrum_viewer_reference_name
).get(msg.data.label)
viewer = self.app.get_viewer(self._default_spectrum_viewer_reference_name)
statistic = getattr(viewer, 'function', None)
viewer_data = self.app._jdaviz_helper.get_data(data_label=msg.data.label,
statistic=statistic)

# If no data is currently plotted, don't attempt to update
if viewer_data is None:
Expand Down Expand Up @@ -300,7 +301,7 @@ def _calculate_statistics(self, *args, **kwargs):
Run the line analysis functions on the selected data/subset and
display the results.
"""
if not hasattr(self, 'dataset'):
if not hasattr(self, 'dataset') or self.app._jdaviz_helper is None:
# during initial init, this can trigger before the component is initialized
return

Expand All @@ -314,16 +315,25 @@ def _calculate_statistics(self, *args, **kwargs):
# show spinner with overlay
self.results_computing = True

if (self.config == 'cubeviz' and self.spatial_subset_selected not in
(SPATIAL_DEFAULT_TEXT, "")):
# then we're acting on the auto-collapsed data in the spectrum-viewer
# of a spatial subset. In the future, we may want to expose on-the-fly
# collapse options... but right now these will follow the settings of the
# spectrum-viewer itself
full_spectrum = self.app.get_data_from_viewer(
self._default_spectrum_viewer_reference_name,
self.spatial_subset_selected
)
if self.config == 'cubeviz':
statistic = None
for viewer in self.dataset.viewers:
if hasattr(viewer.state, "function"):
statistic = viewer.state.function
break
subset_to_apply = None
if self.spatial_subset_selected not in (SPATIAL_DEFAULT_TEXT, ""):
# then we're acting on the auto-collapsed data in the spectrum-viewer
# of a spatial subset. In the future, we may want to expose on-the-fly
# collapse options... but right now these will follow the settings of the
# spectrum-viewer itself
subset_to_apply = self.spatial_subset_selected

full_spectrum = self.app._jdaviz_helper.get_data(
data_label=self.dataset.selected,
subset_to_apply=subset_to_apply,
statistic=statistic)

else:
full_spectrum = self.dataset.selected_obj

Expand Down Expand Up @@ -390,10 +400,8 @@ def _calculate_statistics(self, *args, **kwargs):
'right': np.array([sr.upper.value, max(spectral_axis.value[continuum_mask])])}

else:
continuum_mask = ~self.app.get_data_from_viewer(
self._default_spectrum_viewer_reference_name,
data_label=self.continuum_subset_selected
).mask
continuum_mask = ~self.app._jdaviz_helper.get_data(
subset_to_apply=self.continuum_subset_selected).mask
spectral_axis_nanmasked = spectral_axis.value.copy()
spectral_axis_nanmasked[~continuum_mask] = np.nan
if self.spectral_subset_selected == "Entire Spectrum":
Expand Down
10 changes: 5 additions & 5 deletions jdaviz/configs/specviz/plugins/parsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ def specviz_spectrum1d_parser(app, data, data_label=None, format=None, show_in_v
raise ValueError(f"Length of data labels list ({len(data_label)}) is different"
f" than length of list of data ({len(data)})")

# If there's already data in the viewer, convert units if needed
# If there's already visible data in the viewer, convert units if needed
current_unit = None
if spectrum_viewer_reference_name in app.get_viewer_reference_names():
current_spec = app.get_data_from_viewer(spectrum_viewer_reference_name)
if current_spec != {} and current_spec is not None:
spec_key = list(current_spec.keys())[0]
current_unit = current_spec[spec_key].spectral_axis.unit
sv = app.get_viewer(spectrum_viewer_reference_name)
for layer_state in sv.state.layers:
if layer_state.visible:
current_unit = sv.state.x_display_unit

with app.data_collection.delay_link_manager_update():
# these are used to build a combined spectrum with all
Expand Down
4 changes: 2 additions & 2 deletions jdaviz/configs/specviz/tests/test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,13 @@ def test_get_spectra_no_spectra_redshift_error(specviz_helper, spectrum1d):

def test_get_spectra_no_spectra_label(specviz_helper, spectrum1d):
label = "label"
with pytest.raises(AttributeError):
with pytest.raises(ValueError):
specviz_helper.get_spectra(data_label=label)


def test_get_spectra_no_spectra_label_redshift_error(specviz_helper, spectrum1d):
label = "label"
with pytest.raises(AttributeError):
with pytest.raises(ValueError):
specviz_helper.get_spectra(data_label=label, apply_slider_redshift=True)


Expand Down
39 changes: 11 additions & 28 deletions jdaviz/core/template_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -984,30 +984,21 @@ def selected_subset_state(self):

@property
def selected_subset_mask(self):
if self._allowed_type == 'spatial':
viewer_ref = getattr(self.plugin,
'_default_flux_viewer_reference_name',
self.viewers[0].reference_id)
if self._allowed_type == 'spatial' or self.app.config != 'cubeviz':
statistic = None
elif self._allowed_type == 'spectral':
viewer_ref = getattr(self.plugin,
'_default_spectrum_viewer_reference_name',
self.viewers[0].reference_id)
statistic = self.app.get_viewer(viewer_ref).state.function

subset = self.app.get_data_from_viewer(
viewer_ref, data_label=self.selected
)
data_label = self.plugin.dataset.selected

if hasattr(subset, 'mask'):
# the mask attr is available for spectral subsets:
subset_mask = subset.mask
else:
# `subset` is a GroupedSubset.
# We take the logical-not of the mask here, since the glue subset masks
# are True in selected regions and False outside. This is the opposite of
# the numpy/astropy/specutils convention where True is masked-out.
subset_mask = ~subset.to_mask()
subset = self.app._jdaviz_helper.get_data(data_label=data_label,
subset_to_apply=self.selected,
statistic=statistic)

return subset_mask
return subset.mask

def selected_min_max(self, spectrum1d):
if self.selected_obj is None:
Expand Down Expand Up @@ -1420,17 +1411,9 @@ def selected_obj(self):
if self.selected not in self.labels:
# _apply_default_selection will override shortly anyways
return None
for viewer_ref in self.viewer_refs:
if viewer_ref is None:
# image viewers might not have a reference, but get_data_from_viewer
# does not take id
continue
match = self.app.get_data_from_viewer(viewer_ref, data_label=self.selected)
if match is not None:
if hasattr(match, 'get_object'):
# cube viewers return the data collection instance from get_data_from_viewer
return match.get_object(cls=self.default_data_cls)
return match
match = self.app._jdaviz_helper.get_data(data_label=self.selected)
if match is not None:
return match
# handle the case of empty Application with no viewer, we'll just pull directly
# from the data collection
return self.get_object(cls=self.default_data_cls)
Expand Down