Fix NaN handling in cube fitting #3191

rosteen · 2024-09-12T14:06:08Z

I split this out from #3190 so it can be backported to 3.10. Still need to add a test and move the changelog.

Edit: ready for review. Note that simply adding filter_non_finite to the specutils.fit_lines call like in the 1D case didn't seem to work, so I masked out the NaN locations instead. Why that didn't work might be worth further investigation in the future.

Remove debugging prints, add comment for context Codestyle, changelog Remove unit conversion stuff to just fix NaN handling

codecov · 2024-09-12T14:34:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.46%. Comparing base (88ff8da) to head (f1e7898).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3191      +/-   ##
==========================================
+ Coverage   88.45%   88.46%   +0.01%     
==========================================
  Files         124      124              
  Lines       18389    18394       +5     
==========================================
+ Hits        16266    16273       +7     
+ Misses       2123     2121       -2

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

pllim · 2024-09-12T15:43:44Z

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

@@ -190,7 +190,7 @@ def initialize(self, instance, x, y):

        # width can be estimated by the weighted
        # 2nd moment of the X coordinate.
-        dx = x - np.mean(x)
+        dx = x - np.nanmean(x)


But np.nanmean would still return NaN if x is all NaNs. Is this something we need to handle?

Honestly this probably didn't need to change to nanmean since there shouldn't be NaN x (spectral axis) values. The y_mean case above is the important one.

Same question for y also.

I don't know why someone would try to fit a cube of all NaNs but I think it's reasonable for it to not work if they do 🤷

But didn't you say multiprocessing would swallow the exception, causing confusion?

I could imagine someone did a bad subset that encompass all NaNs in the cube and then try to fit it. It is possible?

pllim · 2024-09-12T15:44:21Z

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

@@ -917,6 +917,12 @@ def _fit_model_to_cube(self, add_data):

        # Apply masks from selected spectral subset
        spec = self._apply_subset_masks(spec, self.spectral_subset)
+        # Also mask out NaNs for fitting. Simply adding filter_non_finite to the cube fit
+        # didn't work out of the box, so doing this for now.
+        if spec.mask is None:


Is this new logic tested?

The None case is, I should probably add a case to test the combination of existing mask and Nan-masking.

I added a test that should cover the existing spec.mask case as well (that mask would come from a spectral subset).

rosteen · 2024-09-12T19:18:11Z

I have no idea why tests are failing on 3.11 and not 3.10 or 3.12, I'll need to investigate.

Also note that I was initially testing the spec.mask combined with NaN-masking case by loading a Spectrum1D with a mask attribute before realizing that that loading case is not supported right now - I fixed the first couple bugs I ran into with that, but stopped before going to far down the rabbit hole.

rosteen · 2024-09-12T20:16:19Z

The tests are passing locally for me on python 3.11, I don't see any package version differences that should affect them...I restarted the test and am crossing my fingers.

pllim · 2024-09-12T20:31:09Z

Hmm still fails. Could it be floating point differences? Maybe use allclose instead of equality? Also did you run with --remote-data --run-slow locally?

pllim · 2024-09-12T20:32:23Z

Try change

assert np.all(result.get_component("flux").data == 1)

to

np.testing.assert_allclose(result.get_component("flux").data, 1)

❓

rosteen · 2024-09-12T20:39:58Z

Try change

assert np.all(result.get_component("flux").data == 1)

to

np.testing.assert_allclose(result.get_component("flux").data, 1)

❓

No, you can see they're all 0s (as was the case before I fixed it) in that test environment rather than all 1s.

I just confirmed that it also fails on my local WSL Ubuntu install, while still passing with an env set up the same way on my Mac, so at least I can reproduce it...

pllim · 2024-09-12T20:44:46Z

also fails on my local WSL Ubuntu install, while still passing with an env set up the same way on my Mac

🤯

What do we have that can possibly be so OS specific?

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

…g.py Co-authored-by: P. L. Lim <[email protected]>

rosteen · 2024-09-12T20:51:56Z

Locally the tests pass on Ubuntu if I wrap the fitting call in the test to ignore the AstropyUserWarning about linear fitting, even though that's not what the logs were showing as the error 🤯

pllim · 2024-09-12T21:06:01Z

Looks like warning is not always emitted.

E       Failed: DID NOT WARN. No warnings of type (<class 'AstropyUserWarning'>,) were emitted.
E        Emitted warnings: [].

Maybe just a blind ignore instead of pytest.warn

with warnings.catch_warnings():
    warnings.filterwarnings(...)

rosteen · 2024-09-12T21:09:09Z

Maybe just a blind ignore instead of pytest.warn
with warnings.catch_warnings():
    warnings.filterwarnings(...)

Yup I just realized that's what was in the test above these.

pllim

Assuming CI is green this time, code changes look reasonable. Thanks!

rosteen · 2024-09-12T21:17:11Z

Whew, looks like that did it. Passing locally too.

gibsongreen

I've been following all the comments, and all the new tests look good and give the result I'd expect in a notebook, great work finishing this one off!

lumberbot-app · 2024-09-13T14:41:44Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.10.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 d26971f50716fe9210756bf251d28720adcbc21f

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #3191: Fix NaN handling in cube fitting'

Push to a named branch:

git push YOURFORK v3.10.x:auto-backport-of-pr-3191-on-v3.10.x

Create a PR against branch v3.10.x, I would have named this PR:

"Backport PR #3191 on branch v3.10.x (Fix NaN handling in cube fitting)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

pllim · 2024-09-13T21:23:56Z

@rosteen is this worth backporting manually? If not, please update the milestone. And we also need follow-up PR to move the change log. Thanks!

Fix NaN handling in cube fitting

fefd283

Remove debugging prints, add comment for context Codestyle, changelog Remove unit conversion stuff to just fix NaN handling

rosteen added bug Something isn't working cubeviz plugin Label for plugins common to multiple configurations 💤backport-v3.10.x on-merge: backport to v3.10.x labels Sep 12, 2024

rosteen added this to the 3.10.4 milestone Sep 12, 2024

Move changelog

db5ed01

rosteen added 2 commits September 12, 2024 11:29

Add test

7cec815

Codestyle

6511530

rosteen marked this pull request as ready for review September 12, 2024 15:32

rosteen requested review from javerbukh, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners September 12, 2024 15:32

pllim reviewed Sep 12, 2024

View reviewed changes

rosteen added 5 commits September 12, 2024 13:20

Adding test for spectrum with existing mask

4252e8a

Fix a couple bugs with a loading a masked cube

163e3dd

Linking fix for case with extra components like mask

95f9fdb

Test mask addition behavior with a subset instead

3832873

Codestyle

766df6a

pllim reviewed Sep 12, 2024

View reviewed changes

jdaviz/configs/default/plugins/model_fitting/tests/test_fitting.py Outdated Show resolved Hide resolved

rosteen and others added 2 commits September 12, 2024 16:48

Ignore UserWarning for Ubuntu tests

ebd3572

Update jdaviz/configs/default/plugins/model_fitting/tests/test_fittin…

862dd87

…g.py Co-authored-by: P. L. Lim <[email protected]>

Do this as in above test

f1e7898

pllim approved these changes Sep 12, 2024

View reviewed changes

gibsongreen approved these changes Sep 13, 2024

View reviewed changes

rosteen merged commit d26971f into spacetelescope:main Sep 13, 2024
19 checks passed

lumberbot-app bot added the Still Needs Manual Backport label Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NaN handling in cube fitting #3191

Fix NaN handling in cube fitting #3191

rosteen commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

pllim Sep 12, 2024

rosteen Sep 12, 2024

pllim Sep 12, 2024

rosteen Sep 12, 2024

pllim Sep 12, 2024

pllim Sep 12, 2024

pllim Sep 12, 2024

rosteen Sep 12, 2024

rosteen Sep 12, 2024 •

edited

Loading

rosteen commented Sep 12, 2024 •

edited

Loading

rosteen commented Sep 12, 2024

pllim commented Sep 12, 2024

pllim commented Sep 12, 2024

rosteen commented Sep 12, 2024

pllim commented Sep 12, 2024

rosteen commented Sep 12, 2024 •

edited

Loading

pllim commented Sep 12, 2024

rosteen commented Sep 12, 2024

pllim left a comment

rosteen commented Sep 12, 2024

gibsongreen left a comment

lumberbot-app bot commented Sep 13, 2024

pllim commented Sep 13, 2024

Fix NaN handling in cube fitting #3191

Fix NaN handling in cube fitting #3191

Conversation

rosteen commented Sep 12, 2024 • edited Loading

codecov bot commented Sep 12, 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

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 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

rosteen commented Sep 12, 2024 • edited Loading

rosteen commented Sep 12, 2024

pllim commented Sep 12, 2024

pllim commented Sep 12, 2024

rosteen commented Sep 12, 2024

pllim commented Sep 12, 2024

rosteen commented Sep 12, 2024 • edited Loading

pllim commented Sep 12, 2024

rosteen commented Sep 12, 2024

pllim left a comment

Choose a reason for hiding this comment

rosteen commented Sep 12, 2024

gibsongreen left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Sep 13, 2024

pllim commented Sep 13, 2024

rosteen commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

rosteen Sep 12, 2024 •

edited

Loading

rosteen commented Sep 12, 2024 •

edited

Loading

rosteen commented Sep 12, 2024 •

edited

Loading