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

Export spectral subsets #2843

Merged
merged 17 commits into from
May 2, 2024
Merged
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ New Features

- Adding Data Quality plugin for Imviz and Cubeviz. [#2767, #2817]

- Enable exporting spectral regions to ECSV files readable by ``astropy.table.QTable`` or
``specutils.SpectralRegion`` [#2843]

Cubeviz
^^^^^^^

Expand Down
57 changes: 51 additions & 6 deletions jdaviz/configs/default/plugins/export/export.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,6 @@
self.dataset.filters = ['is_not_wcs_only', 'not_child_layer',
'from_plugin']

# NOTE: if/when adding support for spectral subsets, update the languange in the UI
self.subset.filters = ['is_spatial']

viewer_format_options = ['png', 'svg']
if self.config == 'cubeviz':
if not self.app.state.settings.get('server_is_remote'):
Expand All @@ -135,7 +132,9 @@
selected='plugin_table_format_selected',
manual_options=plugin_table_format_options)

subset_format_options = ['fits', 'reg']
subset_format_options = [{'label': 'fits', 'value': 'fits', 'disabled': False},
{'label': 'reg', 'value': 'reg', 'disabled': False},
{'label': 'ecsv', 'value': 'ecsv', 'disabled': True}]
self.subset_format = SelectPluginComponent(self,
items='subset_format_items',
selected='subset_format_selected',
Expand Down Expand Up @@ -226,6 +225,8 @@
if name != attr:
setattr(self, attr, '')
if attr == 'subset_selected':
if self.subset.selected != '':
self._update_subset_format_disabled()
self._set_subset_not_supported_msg()
if attr == 'dataset_selected':
self._set_dataset_not_supported_msg()
Expand Down Expand Up @@ -258,6 +259,42 @@
# Clear overwrite warning when user changes filename
self.overwrite_warn = False

def _update_subset_format_disabled(self):
new_items = []
if self.subset.selected is not None:
subset = self.app.get_subsets(self.subset.selected)
if self.app._is_subset_spectral(subset[0]):
good_formats = ["ecsv"]
else:
good_formats = ["fits", "reg"]
for item in self.subset_format_items:
if item["label"] in good_formats:
item["disabled"] = False
else:
item["disabled"] = True
if item["label"] == self.subset_format.selected:
self.subset_format.selected = good_formats[0]

Check warning on line 276 in jdaviz/configs/default/plugins/export/export.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/default/plugins/export/export.py#L276

Added line #L276 was not covered by tests
new_items.append(item)
self.subset_format_items = []
self.subset_format_items = new_items

@observe('subset_format_selected')
def _disable_subset_format_combo(self, event):
# Disable selecting a bad subset+format combination from the API
if self.subset.selected == '' or self.subset.selected is None:
return
subset = self.app.get_subsets(self.subset.selected)
bad_combo = False
if self.app._is_subset_spectral(subset[0]):
if event['new'] != "ecsv":
bad_combo = True

Check warning on line 290 in jdaviz/configs/default/plugins/export/export.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/default/plugins/export/export.py#L289-L290

Added lines #L289 - L290 were not covered by tests
elif event['new'] == "ecsv":
bad_combo = True

if bad_combo:
raise ValueError(f"Cannot export {self.subset.selected} in "
f"{self.subset_format.selected.upper()} format")
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't revert the actual value, and so leaves the plugin in a broken state at which point clicking export or calling export() gives an uninformative traceback. I suspect the code to revert the selection would be messy and perhaps confusing behavior, but maybe we can keep this error message, still show the red text and disable the export button, and then raise that red text as an error on export (all that infrastructure should already exist for the other cases already 🤞 )

To reproduce:

  • create a spectral subset and select in the UI
  • using the API, request 'fits' as the format. This error is raised but the value is still set.
  • (subsequent attempts to set would not raise an error since the value hasn't changed)
  • click or call export


def _set_subset_not_supported_msg(self, msg=None):
"""
Check if selected subset is spectral or composite, and warn and
Expand All @@ -269,7 +306,7 @@
if self.subset.selected == '':
self.subset_invalid_msg = ''
elif self.app._is_subset_spectral(subset[0]):
self.subset_invalid_msg = 'Export for spectral subsets not yet supported.'
self.subset_invalid_msg = ''
elif len(subset) > 1:
self.subset_invalid_msg = 'Export for composite subsets not yet supported.'
else:
Expand Down Expand Up @@ -422,7 +459,11 @@
if raise_error_for_overwrite:
raise FileExistsError(f"{filename} exists but overwrite=False")
return
self.save_subset_as_region(selected_subset_label, filename)

if self.subset_format.selected in ('fits', 'reg'):
self.save_subset_as_region(selected_subset_label, filename)
elif self.subset_format.selected == 'ecsv':
self.save_subset_as_table(filename)

elif len(self.dataset.selected):
filetype = self.dataset_format.selected
Expand Down Expand Up @@ -655,6 +696,10 @@

region.write(filename, overwrite=True)

def save_subset_as_table(self, filename):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want an option to overwrite? Also does this need some path wrangling magic for the "standalone app"? For example, see implementation in this method:

def _write_moment_to_fits(self, overwrite=False, *args):

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I see that this plugin already has _normalize_filename method that you wrote recently. Should that be used explicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_normalize_filename is already used before we get to this point, and handling overwrite is also handled in that method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK, hard to tell from the diff. Thanks for the clarification!

region = self.app.get_subsets(subset_name=self.subset.selected)
region.write(filename)

def vue_interrupt_recording(self, *args): # pragma: no cover
self.movie_interrupt = True
# TODO: this will need updating when batch/multiselect support is added
Expand Down
14 changes: 8 additions & 6 deletions jdaviz/configs/default/plugins/export/export.vue
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
:items="viewer_format_items.map(i => i.label)"
label="Format"
hint="Image format for exporting viewers."
:disabled="viewer_selected.length == 0"
:disabled="viewer_selected.length == 0"
persistent-hint
>
</v-select>
Expand Down Expand Up @@ -126,7 +126,7 @@
</div>

<div v-if="subset_items.length > 0">
<span class="export-category">Spatial Subsets</span>
<span class="export-category">Subsets</span>
<v-row>
<span class="category-content v-messages v-messages__message text--secondary">Export spatial subset as astropy region.</span>
Copy link
Member

Choose a reason for hiding this comment

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

this description definitely needs generalizing now (while still keeping this information since that was explicitly requested).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, good catch. I'll update.

</v-row>
Expand All @@ -150,7 +150,9 @@
:menu-props="{ left: true }"
attach
v-model="subset_format_selected"
:items="subset_format_items.map(i => i.label)"
:items="subset_format_items"
item-text="label"
item-disabled="disabled"
label="Format"
hint="Format for exporting subsets."
:disabled="subset_selected == null || subset_selected.length == 0"
Expand Down Expand Up @@ -200,10 +202,10 @@
:single_select_allow_blank="false"
>
</plugin-inline-select>
<jupyter-widget
<jupyter-widget
v-if='plugin_plot_selected_widget.length > 0'
style="position: absolute; left: -100%"
:widget="plugin_plot_selected_widget"/>
:widget="plugin_plot_selected_widget"/>
<v-row class="row-min-bottom-padding">
<v-select
class="category-content"
Expand Down Expand Up @@ -297,7 +299,7 @@
display: block;
text-align: center;
overflow: hidden;
white-space: nowrap;
white-space: nowrap;
text-transform: uppercase;
color: gray;
font-weight: 500;
Expand Down
24 changes: 18 additions & 6 deletions jdaviz/configs/default/plugins/export/tests/test_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def test_basic_export_subsets_imviz(self, imviz_helper):

# test that invalid file extension raises an error
with pytest.raises(ValueError,
match=re.escape("x not one of ['fits', 'reg'], reverting selection to reg")): # noqa
match=re.escape("x not one of ['fits', 'reg', 'ecsv'], reverting selection to reg")): # noqa
export_plugin.subset_format.selected = 'x'

def test_not_implemented(self, cubeviz_helper, spectral_cube_wcs):
Expand All @@ -93,10 +93,6 @@ def test_not_implemented(self, cubeviz_helper, spectral_cube_wcs):

assert export_plugin.subset_invalid_msg == 'Export for composite subsets not yet supported.'

cubeviz_helper.app.session.edit_subset_mode.mode = NewMode
cubeviz_helper.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(5, 15.5))
assert 'Subset 2' not in export_plugin.subset.choices
Copy link
Contributor

Choose a reason for hiding this comment

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

What does deleting this from the test mean in terms of expected behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It means Subset 2 is allowed to be in the subset choices now.

Copy link
Contributor

Choose a reason for hiding this comment

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

But is it grayed out? If so, is there a way to check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not grayed out, no. The output format choices may be, I can add a check for that.


def test_export_subsets_wcs(self, imviz_helper, spectral_cube_wcs):

# using cube WCS instead of 2d imaging wcs for consistancy with
Expand Down Expand Up @@ -193,9 +189,13 @@ def test_basic_export_subsets_cubeviz(self, cubeviz_helper, spectral_cube_wcs):

# test that invalid file extension raises an error
with pytest.raises(ValueError,
match=re.escape("x not one of ['fits', 'reg'], reverting selection to reg")): # noqa
match=re.escape("x not one of ['fits', 'reg', 'ecsv'], reverting selection to reg")): # noqa
export_plugin.subset_format.selected = 'x'

# Test that selecting disabled option raises an error
with pytest.raises(ValueError, match="Cannot export Subset 1 in ECSV format"):
export_plugin.subset_format.selected = 'ecsv'

# test that attempting to save a composite subset raises an error
cubeviz_helper.app.session.edit_subset_mode.mode = AndMode
cubeviz_helper.app.get_viewer('flux-viewer').apply_roi(CircularROI(xc=25, yc=25, radius=5))
Expand All @@ -205,6 +205,18 @@ def test_basic_export_subsets_cubeviz(self, cubeviz_helper, spectral_cube_wcs):
match='Subset can not be exported - Export for composite subsets not yet supported.'): # noqa
export_plugin.export()

# Test saving spectral subset
cubeviz_helper.app.session.edit_subset_mode.mode = NewMode
cubeviz_helper.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(5, 15.5))
export_plugin.subset.selected = 'Subset 2'

# Format should auto-update to first non-disabled entry
assert export_plugin.subset_format.selected == 'ecsv'

export_plugin.filename_value = "test_spectral_region"
export_plugin.export()
assert os.path.isfile('test_spectral_region.ecsv')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is roundtripping a requirement? That is, is Jdaviz expected to be able to re-ingest this file and re-creates the same spectral region?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't believe it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I guess checking for the output content is correct or not is the job of specutils, so we don't have to do that here. Thanks for the clarification!



@pytest.mark.usefixtures('_jail')
def test_export_cubeviz_spectrum_viewer(cubeviz_helper, spectrum1d_cube):
Expand Down
2 changes: 1 addition & 1 deletion jdaviz/core/template_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ def __init__(self, *args, **kwargs):
manual_options = [default_text] + manual_options
self._manual_options = manual_options

self.items = [{"label": opt} for opt in manual_options]
self.items = [{"label": opt} if isinstance(opt, str) else opt for opt in manual_options]
# set default values for traitlets
if default_text is not None:
self.selected = default_text
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ dependencies = [
"ipywidgets>=8.0.6",
"voila>=0.4,<0.5",
"pyyaml>=5.4.1",
"specutils>=1.9",
"specutils>=1.15",
Copy link
Contributor

Choose a reason for hiding this comment

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

That is quite a jump. Do we need a change log for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would be happy to add it to the change log if you think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't hurt?

"specreduce>=1.3.0,<1.4.0",
"photutils>=1.4",
"glue-astronomy>=0.10",
Expand Down