add fit_non_finite flag to model_fitting 1D #2437

cshanahan1 · 2023-09-11T14:08:44Z

This PR passes the filter_non_finite flag (set to True), to specutils.fitting.fit_lines. This allows non-finite values in the uncertainty/weights array to be filtered out before fitting, allowing the fit to proceed without NonFiniteValueError.

Closes #2328

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

codecov · 2023-09-11T14:23:03Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
...s/default/plugins/model_fitting/fitting_backend.py	`84.00% <100.00%> (ø)`
...igs/default/plugins/model_fitting/model_fitting.py	`86.57% <100.00%> (ø)`
...default/plugins/model_fitting/tests/test_plugin.py	`99.60% <100.00%> (+0.03%)`	⬆️
jdaviz/core/template_mixin.py	`90.92% <96.00%> (+0.09%)`	⬆️
jdaviz/configs/specviz/plugins/parsers.py	`90.56% <94.44%> (-0.24%)`	⬇️

... and 3 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

kecnry · 2023-09-11T14:29:32Z

Do we still want an in-plugin warning when there are detected non-finite values that this would affect?

cshanahan1 · 2023-09-11T14:34:48Z

Do we still want an in-plugin warning when there are detected non-finite values that this would affect?

an AstropyUserWarning will be raised, indicating that non-finite values have been removed from the data before fitting. I was considering adding some additional warnings and if you think that would be useful I can

kecnry · 2023-09-11T14:36:53Z

that would get buried in the standalone mode, so I think we should try to detect in advance (if it isn't too difficult or add too much computation expense) that there are non-finite values in the selected data/subset and just show some text in the plugin saying they will be ignored.

cshanahan1 · 2023-09-11T14:39:20Z

that would get buried in the standalone mode, so I think we should try to detect in advance (if it isn't too difficult or add too much computation expense) that there are non-finite values in the selected data/subset and just show some text in the plugin saying they will be ignored.

ok in that case I have that working in #2429, so I can apply those changes here too. I wasn't sure what the syntax was for displaying a snackbar message here though, who would the 'sender' be in this case? i couldn't find an example in the package of this being done within a function and not a class

kecnry · 2023-09-11T14:43:28Z

I don't think we want a snackbar unless we can't detect the values in advance and can only catch the warning and pass it on to the user instead.

kecnry

Just a few comments from a quick skim. Are there any test cases we can add without introducing large data sets to the tests?

jdaviz/configs/default/plugins/model_fitting/model_fitting.vue

kecnry · 2023-09-21T13:54:44Z

jdaviz/configs/specviz/plugins/parsers.py

@@ -41,8 +44,9 @@ def specviz_spectrum1d_parser(app, data, data_label=None, format=None, show_in_v
        raise TypeError("SpectrumCollection detected."
                        " Please provide a Spectrum1D or SpectrumList")
    elif isinstance(data, Spectrum1D):
-        data = [data]
+        # should we make a copy if Spectrum1D is passed in to avoid modifying it when uncerts are reset to None?


did you test this to see if a copy is needed?

If you pass in a Spectrum1D with uncertainty=[nan, nan, nan...], the input will be modified without a copy, with uncertainty now being None. What do you think about that, is it worth making a copy to avoid modifying input? Im not sure how it works elsewhere in the package if any modifications are made to the input spectrum

jdaviz/configs/specviz/plugins/parsers.py

jdaviz/core/template_mixin.py

cshanahan1 · 2023-09-22T18:35:07Z

I think i responded to all of your comments @kecnry. The only open question is should we be making copies so that the input Spectrum1D is not modified? I added some basic tests (which you can use to demo this, as well as the original dataset when you reported this issue @javerbukh )

javerbukh · 2023-09-22T20:42:14Z

jdaviz/configs/default/plugins/model_fitting/model_fitting.vue

+          <v-row v-if="non_finite_uncertainty_mismatch">
+            <span class="v-messages v-messages__message text--secondary" style="color: red !important">
+                Finite values in the selected data will be omitted in the fit due to corresponding non-finite uncertanties.
+            </span>
+          </v-row>
+


I'm not seeing this in the plugin. How can I make it appear?

uncertainty=StdDevUncertainty([1, 1, np.nan, np.nan, np.nan, np.nan]*u.Jy) spec = Spectrum1D(flux=[1, 2, 3, 4, 5, 6]*u.Jy, uncertainty=uncertainty) specviz = Specviz() specviz.load_data(spec) specviz.show()

that should trigger it. it will appear just above the 'fit model' button.

I tried following the steps of the original ticket and I see the snackbar message "All uncertanties are nonfinite, setting to None". So if no finite values are detected, the uncertainty is set to None. If some non-finite values are detected, this message should show up in the plugin. Do I understand correctly?

I tried and that does in fact make the message appear, thanks!

yep, if the whole array is none then the first snackbar message appears. if its not and there are some non finite uncertainties, then the message in the plugin is displayed

javerbukh · 2023-09-25T12:43:02Z

jdaviz/core/template_mixin.py

+    # note! the 'n/a' traitlet doesn't exist. we are watching it here because
+    # DatasetSpectralSubsetValidMixin is also watching the exact set
+    # of real traitlets, which creates a bug where only the last inhereted method
+    # will run, so we need to spoof it like this.
+    @observe("n/a", "dataset_selected", "spectral_subset_selected")


Are you sure observing "n/a" is necessary? ~~I don't understand what you mean by "watching the exact set of real traitlets", maybe someone can explain it to me if I'm missing something?~~ I see what you mean by real traitlets, dataset_selected and spectral_subset_selected. Still not sure about "n/a".

Just observing "dataset_selected" and "spectral_subset_selected" would conflict with another observe and only the last one added would actually be called. @cshanahan1 - is there an issue for this yet that we can reference?

will post the issue in a sec, i was getting an example code snippet working to create the issue

Has this been resolved? I also don't understand what "n/a" is for.

i think, somehow miraculously, im not seeing this anymore so i've removed the dummy traitlet.

kecnry

Sorry for a ton of comments, but they're all quite minor - overall this seems to work well and the code is quite clean, thanks!

kecnry · 2023-09-25T12:45:32Z

CHANGES.rst

@@ -58,6 +58,14 @@ Specviz2d
 Other Changes and Additions
 ---------------------------

+- Better handling of non-finite uncertainties in model fitting. The
+  'filter_non_finite' flag (for the LevMarLSQFitter) now filters datapoints with 
+  non-finite weights. In Specviz, if a fully-finite spectrum with non-finite


I wonder if this would ever happen for a cube in cubeviz and if its worth generalizing to apply to that case as well (perhaps as part of an eventual parser refactor - not necessarily in this PR)... thoughts?

i think so, we should open a follow up ticket. we would need to move some things (like get_spectrum_1d) to somewhere more general to make this work

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

jdaviz/configs/default/plugins/model_fitting/model_fitting.vue

jdaviz/configs/specviz/plugins/parsers.py

kecnry · 2023-09-25T13:00:42Z

jdaviz/core/template_mixin.py

+    # note! the 'n/a' traitlet doesn't exist. we are watching it here because
+    # DatasetSpectralSubsetValidMixin is also watching the exact set
+    # of real traitlets, which creates a bug where only the last inhereted method
+    # will run, so we need to spoof it like this.
+    @observe("n/a", "dataset_selected", "spectral_subset_selected")


Just observing "dataset_selected" and "spectral_subset_selected" would conflict with another observe and only the last one added would actually be called. @cshanahan1 - is there an issue for this yet that we can reference?

jdaviz/core/template_mixin.py

jdaviz/configs/specviz/plugins/parsers.py

jdaviz/core/template_mixin.py

kecnry

LGTM (definitely qualifies for squashing though)!

cshanahan1 · 2023-09-28T11:36:25Z

LGTM (definitely qualifies for squashing though)!

will do!

jdaviz/core/template_mixin.py

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

jdaviz/configs/specviz/plugins/parsers.py

pllim · 2023-09-28T14:29:20Z

jdaviz/configs/specviz/plugins/parsers.py

+        if uncerts is not None:
+            if not np.any(uncerts):
+                uncerts = None
+                set_nans_to_none = True


Can combine nested if?

Suggested change

if uncerts is not None:

if not np.any(uncerts):

uncerts = None

set_nans_to_none = True

if uncerts is not None and not np.any(uncerts):

uncerts = None

set_nans_to_none = True

pllim · 2023-09-28T14:30:56Z

jdaviz/configs/specviz/plugins/parsers.py

+            # otherwise, data should be a list containing a single [Spectrum1D]
+            # check just to be safe.
+            m = 'Only supported for one data entry, if multiple spectra use SpectrumList.'
+            raise NotImplementedError(m)


Shouldn't this be checked right at the beginning before all the extra processing is done?

done, moved this to the top

pllim · 2023-09-28T14:32:07Z

jdaviz/configs/specviz/plugins/parsers.py

+        msg = 'All uncertainties are nonfinite, replacing with uncertainty=None.'
+        info_msg = SnackbarMessage(msg, color="warning", sender=app)
+        app.hub.broadcast(info_msg)


I have been shortening old calls like this one to one line. No reason to assign 2 variables to this one snackbar call, as they will never be used again.

Suggested change

msg = 'All uncertainties are nonfinite, replacing with uncertainty=None.'

info_msg = SnackbarMessage(msg, color="warning", sender=app)

app.hub.broadcast(info_msg)

msg = 'All uncertainties are nonfinite, replacing with uncertainty=None.'

info_msg = SnackbarMessage(msg, color="warning", sender=app)

app.hub.broadcast(info_msg)

i shortened this to two lines, i thought it was nicer looking than splitting the string over two lines

jdaviz/core/template_mixin.py

CHANGES.rst

jdaviz/configs/specviz/plugins/parsers.py

jdaviz/core/template_mixin.py

pllim

Missed some doc indentation.

Otherwise I think in general the diff LGTM, so @javerbukh can do his review now. Thanks for your patience!

jdaviz/core/template_mixin.py

pllim · 2023-09-28T16:59:05Z

p.s. Open a new #2487 for general clean-up of the parser, if we ever get to it in the future.

javerbukh

LGTM!

cshanahan1 · 2023-09-28T17:45:46Z

ok thanks everyone! im going to merge this once CI passes

cshanahan1 requested review from rosteen, javerbukh, pllim, kecnry, bmorris3 and haticekaratay as code owners September 11, 2023 14:08

pllim reviewed Sep 11, 2023

View reviewed changes

jdaviz/configs/default/plugins/model_fitting/fitting_backend.py Show resolved Hide resolved

cshanahan1 mentioned this pull request Sep 15, 2023

reset fully non-finite uncertainties to None in specviz parser #2429

Closed

9 tasks

cshanahan1 force-pushed the nonfinite_flag branch from 3a5116b to 24a2205 Compare September 21, 2023 13:49

kecnry reviewed Sep 21, 2023

View reviewed changes

cshanahan1 force-pushed the nonfinite_flag branch from ba37cb8 to a5298c6 Compare September 22, 2023 17:43

kecnry added this to the 3.8 milestone Sep 22, 2023

javerbukh reviewed Sep 22, 2023

View reviewed changes

javerbukh reviewed Sep 25, 2023

View reviewed changes

kecnry reviewed Sep 25, 2023

View reviewed changes

kecnry reviewed Sep 27, 2023

View reviewed changes

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

cshanahan1 force-pushed the nonfinite_flag branch from 15ff2ca to 854389f Compare September 27, 2023 18:29

kecnry reviewed Sep 27, 2023

View reviewed changes

jdaviz/configs/specviz/plugins/parsers.py Outdated Show resolved Hide resolved

jdaviz/configs/specviz/plugins/parsers.py Outdated Show resolved Hide resolved

jdaviz/configs/specviz/plugins/parsers.py Show resolved Hide resolved

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved

kecnry approved these changes Sep 28, 2023

View reviewed changes