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.

Sign up for GitHub

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

Re-enable exporting cubeviz spectrum viewer #2825

Merged
merged 3 commits into from
Apr 24, 2024
Merged

Re-enable exporting cubeviz spectrum viewer #2825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
479d17f
enable option to export spectrum viewer in cubeviz
kecnry Apr 23, 2024
efd9240
temporarily "clean" incompatible marks
kecnry Apr 23, 2024
929ec1e
test coverage
kecnry Apr 23, 2024
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
2 changes: 2 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ Bug Fixes
Cubeviz
^^^^^^^

- Re-enable support for exporting spectrum-viewer. [#2825]

Imviz
^^^^^

Expand Down
33 changes: 22 additions & 11 deletions jdaviz/configs/default/plugins/export/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from glue_jupyter.bqplot.image import BqplotImageView

from jdaviz.core.custom_traitlets import FloatHandleEmpty, IntHandleEmpty
from jdaviz.core.marks import ShadowMixin
from jdaviz.core.registries import tray_registry
from jdaviz.core.template_mixin import (PluginTemplateMixin, SelectPluginComponent,
ViewerSelectMixin, DatasetMultiSelectMixin,
Expand Down Expand Up @@ -221,17 +222,6 @@ def _sync_singleselect(self, event):
self._set_subset_not_supported_msg()
if attr == 'dataset_selected':
self._set_dataset_not_supported_msg()
elif self.config == "cubeviz" and attr == "viewer_selected":
self._disable_viewer_format_combo(event)

@observe('viewer_format_selected')
def _disable_viewer_format_combo(self, event):
if (self.config == "cubeviz" and self.viewer_selected == "spectrum-viewer"
and self.viewer_format_selected == "png"):
msg = "Exporting the spectrum viewer as a PNG in Cubeviz is not yet supported"
else:
msg = ""
self.viewer_invalid_msg = msg

@observe('filename')
def _is_filename_changed(self, event):
Expand Down Expand Up @@ -341,11 +331,32 @@ def export(self, filename=None, show_dialog=None, overwrite=False,
raise FileExistsError(f"{filename} exists but overwrite=False")
return

# temporarily "clean" incompatible marks of unicode characters, etc
restores = []
for mark in viewer.figure.marks:
restore = {}
if len(getattr(mark, 'text', [])):
if not isinstance(mark, ShadowMixin):
# if it is shadowing another mark, that will automatically get updated
# when the other mark is restored, but we'll still ensure that the mark
# is clean of unicode before exporting.
restore['text'] = [t for t in mark.text]
mark.text = [t.strip() for t in mark.text]
if len(getattr(mark, 'labels', [])):
restore['labels'] = mark.labels[:]
mark.labels = [lbl.strip() for lbl in mark.labels]
restores.append(restore)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we also have restore_marks and only populate the two lists if restore isn't empty, to avoid looping over things we don't need to update in the zip later?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow - do you mean store a copy of the original mark? I'm trying to store/restore as little as possible to minimize memory impacts. I tried to avoid the append, but there can be some copying-referencing issues if we're not careful then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
restores.append(restore)
if restore != {}:
restores.append(restore)
restore_marks.append(mark)

Would restore_mark have a copy of the object or a reference to the original in this case? I can never keep track of when python does which, but this would avoid looping over things that don't need to be restored. If memory is an issues it can stay as-is, I honestly don't think this is going to be impactful either way here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect this would be more expensive than looping over the empty dictionary, but I'm not sure. We could also track indices, but I'm not sure its worth it either 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I said, I don't feel strongly about it, I'll just approve.


if filetype == "mp4":
self.save_movie(viewer, filename, filetype)
else:
self.save_figure(viewer, filename, filetype, show_dialog=show_dialog)

# restore marks to their original state
for restore, mark in zip(restores, viewer.figure.marks):
for k, v in restore.items():
setattr(mark, k, v)

elif len(self.plugin_plot.selected):
plot = self.plugin_plot.selected_obj._obj
filetype = self.plugin_plot_format.selected
Expand Down
13 changes: 13 additions & 0 deletions jdaviz/configs/default/plugins/export/tests/test_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,19 @@ def test_basic_export_subsets_cubeviz(self, cubeviz_helper, spectral_cube_wcs):
export_plugin.export()


@pytest.mark.usefixtures('_jail')
def test_export_cubeviz_spectrum_viewer(cubeviz_helper, spectrum1d_cube):
cubeviz_helper.load_data(spectrum1d_cube, data_label='test')

ep = cubeviz_helper.plugins["Export"]
ep.viewer = 'spectrum-viewer'
ep.viewer_format = 'png'
ep.export()

ep.viewer_format = 'svg'
ep.export()


@pytest.mark.usefixtures('_jail')
def test_export_data(cubeviz_helper, spectrum1d_cube):
cubeviz_helper.load_data(spectrum1d_cube, data_label='test')
Expand Down