Export spectral subsets #2843

rosteen · 2024-05-01T17:01:05Z

Implements updating spectral subsets via specutils.SpectralRegion to ECSV files.

rosteen · 2024-05-01T17:15:53Z

Hmm, this was passing locally.

…use it in export subset

pllim · 2024-05-01T18:45:06Z

jdaviz/configs/default/plugins/export/tests/test_export.py

@@ -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


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

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

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

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

pllim · 2024-05-01T18:47:04Z

jdaviz/configs/default/plugins/export/tests/test_export.py

+
+        export_plugin.filename_value = "test_spectral_region"
+        export_plugin.export()
+        assert os.path.isfile('test_spectral_region.ecsv')


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

I don't believe it is.

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!

pllim · 2024-05-01T18:47:33Z

pyproject.toml

@@ -23,7 +23,7 @@ dependencies = [
    "ipywidgets>=8.0.6",
    "voila>=0.4,<0.5",
    "pyyaml>=5.4.1",
-    "specutils>=1.9",
+    "specutils>=1.15",


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

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

Doesn't hurt?

codecov · 2024-05-01T18:58:21Z

Codecov Report

Attention: Patch coverage is 97.67442% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.99%. Comparing base (9b4817d) to head (3a415ec).
Report is 10 commits behind head on main.

Files	Patch %	Lines
jdaviz/configs/default/plugins/export/export.py	97.61%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2843      +/-   ##
==========================================
+ Coverage   88.95%   88.99%   +0.03%     
==========================================
  Files         111      111              
  Lines       17101    17148      +47     
==========================================
+ Hits        15213    15261      +48     
+ Misses       1888     1887       -1

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

pllim · 2024-05-01T19:37:55Z

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

@@ -655,6 +696,10 @@ def save_subset_as_region(self, selected_subset_label, filename):

        region.write(filename, overwrite=True)

+    def save_subset_as_table(self, filename):


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:

jdaviz/jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py

Line 314 in ee17ed4

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

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

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

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

pllim

Works as advertised but some feedback for possible future work as follows.

When I have multiple spectral regions, I would like the option to save all or some of them in the same file. Currently, I can only save one Subset per file. But what would be nice is something like this in the same file:

lower_bound upper_bound
7.030608671135324 7.168932092864794
5.701354428693486 5.876789012350375

I have checked that both astropy.table.QTable and specutils.SpectralRegion could read multi-row ECSV back in.

Even better (and probably only makes sense when we support Subset renaming; also double check how ECSV really store fields with whitespace, this is just pseudocode):

name lower_bound upper_bound
"My fav line 1" 7.030608671135324 7.168932092864794
"My other fav line" 5.701354428693486 5.876789012350375

pllim · 2024-05-01T21:31:55Z

p.s. Not sure if there are any other relevant info you want to also store, like the dataset it was tied to, or the redshift. Again, ideas for future work, not this PR.

rosteen · 2024-05-01T21:53:30Z

@pllim Thanks for the comments. This can save out multiple lines to the same file, but it has to be a single subset with multiple subregions rather than multiple independent spectral subsets.

pllim · 2024-05-01T22:03:30Z

Re: #2843 (comment)

Do you think such a distinction should be documented in user doc, maybe at https://jdaviz.readthedocs.io/en/latest/specviz/export_data.html#spectral-regions ? 🤔

kecnry

I was worried about how the automatic changing of format would work wrt the API, but in playing with it, I actually think its ok and quite convenient! One caveat explained below where the API can still get into trouble, but I think we can find ways around that.

I do think it would then be worth looking into what other places this pattern could be used. In some cases, I think the red text is still the better choice because it can explain why, but in cases where it is more obvious like this, this really is a nice UX improvement!

Another thing to consider: instead of bumping specutils, we could just have spectral subsets disabled entirely based on the imported version (in this case it probably would make sense to allow selecting the spectral subset but then have the red text say that upgrading specutils is necessary). I'm ok with the bump though if that keeps things simpler and has other advantages as well.

kecnry · 2024-05-02T13:13:50Z

jdaviz/configs/default/plugins/export/export.vue

@@ -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>


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

Whoops, good catch. I'll update.

kecnry · 2024-05-02T13:24:25Z

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

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


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

rosteen · 2024-05-02T15:56:49Z

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

+            raise ValueError(f"Cannot export {self.subset.selected} in {event['new']}"
+                             f" format, reverting selection to {self.subset_format.selected}")


@kecnry, do you think this error message makes clear enough what is happening?

if you want to go the revert route, you could also use self.subset_format.select_previous() (which hopefully should be valid still... I can't think of a case where it wouldn't)

but the message makes sense to me, let me test it to see if I can find a way to break it and otherwise should be good to approve.

rosteen · 2024-05-02T15:57:42Z

@pllim I updated the docs. It actually looks like the export section in plugins.rst hadn't been updated since we added more things in addition to viewers, so I added a mention of plugin plots as well.

pllim · 2024-05-02T16:03:08Z

Re: docs -- LGTM. Thanks!

docs/specviz/plugins.rst

kecnry

LGTM now - thanks!

rosteen added feature Feature request plugin Label for plugins common to multiple configurations labels May 1, 2024

rosteen added this to the 3.10 milestone May 1, 2024

rosteen requested review from javerbukh, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners May 1, 2024 17:01

rosteen added 11 commits May 1, 2024 14:21

Starting to add spectral subsets to export

31a5550

Update SelectPluginComponent to allow use of disabled item property, …

bdc96c8

…use it in export subset

Add handling for disabling bad subset/format combinations

e999639

Fix codestyle

04654d0

Update tests

60a075d

Add code to actually save the spectral region, add tests

6670683

Update specutils pin

11b8869

Update changelog

7386258

Delete extra line

d8e652f

Debugging tests on CI

2da3ad6

Update to account for recent changes

0fca14b

rosteen force-pushed the export-spectral-subsets branch from 298b52c to 0fca14b Compare May 1, 2024 18:32

pllim reviewed May 1, 2024

View reviewed changes

Address review comments

4e33fba

pllim reviewed May 1, 2024

View reviewed changes

pllim approved these changes May 1, 2024

View reviewed changes

pllim added the Ready for final review label May 1, 2024

kecnry reviewed May 2, 2024

View reviewed changes

rosteen added 2 commits May 2, 2024 11:49

Address review comments

443d93c

Updated docs

47f7c1f

rosteen commented May 2, 2024

View reviewed changes

docs/specviz/plugins.rst Outdated Show resolved Hide resolved

Fix typo

34f5937

rosteen commented May 2, 2024

View reviewed changes

docs/specviz/plugins.rst Outdated Show resolved Hide resolved

rosteen and others added 2 commits May 2, 2024 12:05

typo

c7f7cea

Fix test

3a415ec

kecnry approved these changes May 2, 2024

View reviewed changes

rosteen merged commit 815bce7 into spacetelescope:main May 2, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export spectral subsets #2843

Export spectral subsets #2843

rosteen commented May 1, 2024

rosteen commented May 1, 2024

pllim May 1, 2024

rosteen May 1, 2024

pllim May 1, 2024

rosteen May 1, 2024

pllim May 1, 2024

rosteen May 1, 2024

pllim May 1, 2024

pllim May 1, 2024

rosteen May 1, 2024

pllim May 1, 2024

codecov bot commented May 1, 2024 •

edited

Loading

pllim May 1, 2024

pllim May 1, 2024

rosteen May 1, 2024

pllim May 1, 2024

pllim left a comment

pllim commented May 1, 2024

rosteen commented May 1, 2024 •

edited

Loading

pllim commented May 1, 2024

kecnry left a comment

kecnry May 2, 2024

rosteen May 2, 2024

kecnry May 2, 2024

rosteen May 2, 2024

kecnry May 2, 2024

kecnry May 2, 2024

rosteen commented May 2, 2024

pllim commented May 2, 2024

kecnry left a comment

		@@ -655,6 +696,10 @@ def save_subset_as_region(self, selected_subset_label, filename):

		region.write(filename, overwrite=True)

		def save_subset_as_table(self, filename):

		raise ValueError(f"Cannot export {self.subset.selected} in {event['new']}"
		f" format, reverting selection to {self.subset_format.selected}")

Export spectral subsets #2843

Export spectral subsets #2843

Conversation

rosteen commented May 1, 2024

rosteen commented May 1, 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

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

codecov bot commented May 1, 2024 • edited Loading

Codecov Report

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

pllim commented May 1, 2024

rosteen commented May 1, 2024 • edited Loading

pllim commented May 1, 2024

kecnry left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen commented May 2, 2024

pllim commented May 2, 2024

kecnry left a comment

Choose a reason for hiding this comment

codecov bot commented May 1, 2024 •

edited

Loading

rosteen commented May 1, 2024 •

edited

Loading