prohibit subsets that do not overlap data range within plugins #1935

kecnry · 2022-12-19T20:08:10Z

Description

This pull request prohibits the user selecting a spectral subset that does not overlap at all with the selected dataset. This is done through a new plugin mixin which is then used in both model fitting and line analysis, showing red text in the plugin UIs, disabling the "fit" button in model fitting, and raising a traceback if calling the fit or get_results API methods, respectively.

IMPORTANT NOTE: adding the subset to the right (reference) data will result in a traceback because of #1868. Once that bug is fixed, this logic should then handle that case as well. Notes are added in the test cases if we want to update the test for either line analysis or model fitting to handle the opposite scenario as a regression test for #1868.

Screen.Recording.2022-12-19.at.3.11.07.PM.mov

Fixes #1911

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-12-19T20:26:19Z

Codecov Report

Base: 91.78% // Head: 91.81% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (f26ac6f) compared to base (a2f3ce2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1935      +/-   ##
==========================================
+ Coverage   91.78%   91.81%   +0.03%     
==========================================
  Files         140      140              
  Lines       14989    15045      +56     
==========================================
+ Hits        13758    13814      +56     
  Misses       1231     1231

Impacted Files	Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py	`83.89% <100.00%> (+0.11%)`	⬆️
...default/plugins/model_fitting/tests/test_plugin.py	`100.00% <100.00%> (ø)`
...igs/specviz/plugins/line_analysis/line_analysis.py	`98.08% <100.00%> (+0.04%)`	⬆️
.../plugins/line_analysis/tests/test_line_analysis.py	`99.62% <100.00%> (+0.02%)`	⬆️
jdaviz/core/template_mixin.py	`92.88% <100.00%> (+0.10%)`	⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kecnry · 2022-12-20T16:14:35Z

Unsure whether this should be milestoned as 3.1.2 or 3.2 (especially given the other case covered by #1868), and whether it should be in the changelog as a "feature" or bugfix - opinions/thoughts welcomed!

pllim · 2022-12-20T21:47:40Z

Given we already plan 3.2 for AAS, milestone to 3.2 is probably ok at this point.

pllim

What about partial overlap? What should happen then? Is that tested?

Also on the topic of partial overlap, if it is supported, also see what happens if the overlap is only one element in wavelength space.

pllim · 2022-12-21T13:55:40Z

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

@@ -664,6 +664,9 @@ def calculate_fit(self, add_data=True):
        fitted spectrum/cube
        residuals (if ``residuals_calculate`` is set to ``True``)
        """
+        if not self.spectral_subset_valid:
+            raise ValueError("spectral subset is outside data range")


Would be easier for future debugging if you also report both ranges so people can see why they are not overlapping.

This only ever happens if ignoring the UI warnings and calling from the API directly... although that also means the user likely isn't staring at the viewer where it's visually obvious. I'm not convinced ranges will help though and am guessing the more likely action would be to change one selection or the other... so do you think it would help to be "spectral subset '{self.spectral_subset.selected}' is outside data range of '{self.dataset.selected}'"?

If I am debugging this when user says it should not have errored, I would find it helpful to have the numbers already in the traceback to see why it crashes in the first place and whether that is expected.

ok, I added both. Let me know what you think!

jdaviz/configs/specviz/plugins/line_analysis/line_analysis.py

jdaviz/core/template_mixin.py

pllim · 2022-12-21T14:01:32Z

jdaviz/core/template_mixin.py

+            spec = self.dataset.selected_obj
+            spec_min, spec_max = np.nanmin(spec.spectral_axis), np.nanmax(spec.spectral_axis)
+            subset_min, subset_max = self.spectral_subset.selected_min_max(spec)
+            self.spectral_subset_valid = bool(subset_min < spec_max and subset_max > spec_min)


Why is the range check not inclusive?

Suggested change

self.spectral_subset_valid = bool(subset_min < spec_max and subset_max > spec_min)

self.spectral_subset_valid = bool(subset_min =< spec_max and subset_max >= spec_min)

We need to ensure the subset will contain data. From testing it seemed that isn't the case for exact edge values, but that could just be due to rounding error or might be related to #1892. Can you point to where in the code the subset logic is guaranteed to be inclusive? I don't mind changing this if it should act inclusively, but also want to avoid a rabbit hole here and might just need to revisit this during/after the possible upcoming subset refactoring work.

I don't know on top of my head if it should be inclusive or not. I just want to make sure this was considered. Thanks for the clarification!

kecnry · 2022-12-21T14:22:52Z

What about partial overlap? What should happen then? Is that tested?

Also on the topic of partial overlap, if it is supported, also see what happens if the overlap is only one element in wavelength space.

Partial overlap is the same as a single data set with a small subset on the edge (it does work, 1 element might give you unexpected results when fitting 🤷 , but it won't crash - unless model fitting ever complains about less data points than degrees of freedom, but that is a different case entirely).

pllim

Codewise LGTM and the tests seem to demonstrate that the bug is fixed. Thanks!

* implemented for model fitting and line analysis

bmorris3

Looks good!

Only comment is a minor one. We've mentioned before that we could avoid hard-coding the default text for selected subsets in the future, and I contend that the future is now. I've provided a few suggestions that you can accept/reject below.

bmorris3 · 2023-01-04T19:45:19Z

jdaviz/core/template_mixin.py

+
+    @observe("dataset_selected", "spectral_subset_selected")
+    def _check_dataset_spectral_subset_valid(self, event={}, return_ranges=False):
+        if self.spectral_subset_selected == "Entire Spectrum":


Suggested change

if self.spectral_subset_selected == "Entire Spectrum":

if self.spectral_subset_selected == self.spectral_subset.default_text:

after offline discussion, we'll reconsider this app-wide as a follow-up effort and then try to be consistent after.

bmorris3 · 2023-01-04T19:45:56Z

jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py

+    plugin = specviz_helper.plugins['Line Analysis']
+    plugin.dataset = 'right_spectrum'
+    assert plugin.dataset == 'right_spectrum'
+    assert plugin.spectral_subset == 'Entire Spectrum'


Suggested change

assert plugin.spectral_subset == 'Entire Spectrum'

assert plugin.spectral_subset == plugin.spectral_subset.default_text

bmorris3 · 2023-01-04T19:46:20Z

jdaviz/configs/default/plugins/model_fitting/tests/test_plugin.py

+
+    plugin.dataset = 'right_spectrum'
+    assert plugin.dataset == 'right_spectrum'
+    assert plugin.spectral_subset == 'Entire Spectrum'


Suggested change

assert plugin.spectral_subset == 'Entire Spectrum'

assert plugin.spectral_subset == plugin.spectral_subset.default_text

kecnry added specviz plugin Label for plugins common to multiple configurations labels Dec 19, 2022

kecnry added this to the 3.2 milestone Dec 19, 2022

kecnry force-pushed the model-fitting-subset-must-overlap branch 2 times, most recently from daac88c to da79f65 Compare December 19, 2022 20:14

kecnry marked this pull request as ready for review December 20, 2022 16:25

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino, pllim and bmorris3 as code owners December 20, 2022 16:25

pllim reviewed Dec 21, 2022

View reviewed changes

kecnry force-pushed the model-fitting-subset-must-overlap branch from bfdfebd to 715b52e Compare December 21, 2022 14:55

pllim approved these changes Dec 21, 2022

View reviewed changes

pllim added the Ready for final review label Dec 21, 2022

kecnry force-pushed the model-fitting-subset-must-overlap branch 2 times, most recently from 727a550 to 35c2514 Compare January 3, 2023 15:47

kecnry added 3 commits January 3, 2023 10:52

mixin to prohibit subsets that do not overlap data range

e40c641

* implemented for model fitting and line analysis

test coverage

700608c

add offending ranges to error messages

f26ac6f

kecnry force-pushed the model-fitting-subset-must-overlap branch from 35c2514 to f26ac6f Compare January 3, 2023 15:52

bmorris3 approved these changes Jan 4, 2023

View reviewed changes

kecnry merged commit 0f8317e into spacetelescope:main Jan 4, 2023

kecnry deleted the model-fitting-subset-must-overlap branch January 4, 2023 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prohibit subsets that do not overlap data range within plugins #1935

prohibit subsets that do not overlap data range within plugins #1935

kecnry commented Dec 19, 2022 •

edited

Loading

codecov bot commented Dec 19, 2022 •

edited

Loading

kecnry commented Dec 20, 2022

pllim commented Dec 20, 2022

pllim left a comment

pllim Dec 21, 2022

kecnry Dec 21, 2022

pllim Dec 21, 2022

kecnry Dec 21, 2022

pllim Dec 21, 2022

kecnry Dec 21, 2022

pllim Dec 21, 2022

kecnry commented Dec 21, 2022

pllim left a comment

bmorris3 left a comment

bmorris3 Jan 4, 2023

kecnry Jan 4, 2023

bmorris3 Jan 4, 2023

bmorris3 Jan 4, 2023

	self.spectral_subset_valid = bool(subset_min < spec_max and subset_max > spec_min)
	self.spectral_subset_valid = bool(subset_min =< spec_max and subset_max >= spec_min)

	if self.spectral_subset_selected == "Entire Spectrum":
	if self.spectral_subset_selected == self.spectral_subset.default_text:

	assert plugin.spectral_subset == 'Entire Spectrum'
	assert plugin.spectral_subset == plugin.spectral_subset.default_text

prohibit subsets that do not overlap data range within plugins #1935

prohibit subsets that do not overlap data range within plugins #1935

Conversation

kecnry commented Dec 19, 2022 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Dec 19, 2022 • edited Loading

Codecov Report

kecnry commented Dec 20, 2022

pllim commented Dec 20, 2022

pllim 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

Choose a reason for hiding this comment

kecnry commented Dec 21, 2022

pllim left a comment

Choose a reason for hiding this comment

bmorris3 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

kecnry commented Dec 19, 2022 •

edited

Loading

codecov bot commented Dec 19, 2022 •

edited

Loading