Cubeviz cube fitting: Fix fitted cube all zeroes, allow MP bypass #1333

pllim · 2022-05-20T01:26:55Z

Description

This pull request is to address reported bug where a failed cube fit would silently return a cube with all zeroes because multiprocessing swallowed all the exceptions. This PR intents to:

fix the bug that caused the exception in the first place
allow bypassing multiprocessing when n_cpu is set to 1 (this feature is for developers only)

Fix #1245

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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

codecov · 2022-05-20T01:39:02Z

Codecov Report

Merging #1333 (16ca3d9) into main (2e1e505) will decrease coverage by 0.05%.
The diff coverage is 66.66%.

❗ Current head 16ca3d9 differs from pull request most recent head d55fb5e. Consider uploading reports for the commit d55fb5e to get more accurate results

@@            Coverage Diff             @@
##             main    #1333      +/-   ##
==========================================
- Coverage   84.64%   84.58%   -0.06%     
==========================================
  Files          91       91              
  Lines        7878     7839      -39     
==========================================
- Hits         6668     6631      -37     
+ Misses       1210     1208       -2

Impacted Files	Coverage Δ
jdaviz/utils.py	`89.87% <60.00%> (-2.34%)`	⬇️
...igs/default/plugins/model_fitting/model_fitting.py	`78.79% <100.00%> (ø)`
...default/plugins/metadata_viewer/metadata_viewer.py	`93.40% <0.00%> (-1.10%)`	⬇️
jdaviz/app.py	`91.21% <0.00%> (-1.01%)`	⬇️
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`91.55% <0.00%> (-0.65%)`	⬇️
...z/configs/default/plugins/line_lists/line_lists.py	`68.87% <0.00%> (-0.48%)`	⬇️
jdaviz/configs/mosviz/plugins/parsers.py	`90.42% <0.00%> (-0.14%)`	⬇️
jdaviz/configs/mosviz/helper.py	`86.74% <0.00%> (-0.04%)`	⬇️
jdaviz/core/template_mixin.py	`93.06% <0.00%> (+0.17%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84ddff9...d55fb5e. Read the comment docs.

jdaviz/utils.py

DEV: Allow multiprocessing bypass when n_cpu is 1

pllim

@rosteen , please double check the X and Y order. That part always confuses me because of specutils reordering things and such.

pllim · 2022-05-24T16:51:08Z

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

@@ -119,14 +113,16 @@ def test_cube_fitting_backend():

    SIGMA = 0.1  # noise in data
    TOL = 0.4  # test tolerance
+    IMAGE_SIZE_X = 15


I purposely made this asymmetric so it is easier to tell if we get the spatial dimension wrong.

pllim · 2022-05-24T16:52:08Z

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

    assert fitted_spectrum.flux.unit == u.Jy
+    assert not np.all(fitted_spectrum.flux.value == 0)


Maybe there is a better way to test the contents of fitted_spectrum but maybe we can defer that for the "science verification" campaign.

I don't think it would be too hard to generate a known shape with some noise on top and check that the result is within tolerance of the expected fit, but I agree that I think we can defer that for now.

Should we add a comment above this to the effect of "if this ever fails, set n_cpu=1 to access the tracebacks from fitting"?

There's a comment to that effect on line 149 # n_cpu = 1 # NOTE: UNCOMMENT TO DEBUG LOCALLY, AS NEEDED, think there needs to be another one here?

Instead of copy pasting that sentence everywhere in the test, should I just write it up in dev docs? Any suggestion on where?

this is currently the one place that will catch these failures, right? But I guess we may have more in the future - I'm just trying to save us time in the future if/when this returns zeros and trips this test again. I'm not sure I would remember and see the other comment to even know that a debug mode is an option. But I suppose we always have the blame history back to this PR 🤞

Would it help if I expose this in API doc over at RTD as a follow-up PR?

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

Lines 43 to 48 in 84ddff9

n_cpu : `None` or int

**This is only used for spectral cube fitting.**

Number of cores to use for multiprocessing.

Using all the cores at once is not recommended.

If `None`, it will use max cores minus one.

Set this to 1 for debugging.

I will ping you to review #1346 when it is ready. Thanks!

rosteen

Works for me, thanks.

kecnry

Science verification and regression tests still need to be added (as future efforts) to ensure this bug doesn't resurface for various scenarios, but this at least enables us to write those tests and debug when they fail.

rosteen · 2022-05-25T13:49:29Z

This closes #1245

pllim added the bug Something isn't working label May 20, 2022

pllim added this to the 2.6 milestone May 20, 2022

pllim commented May 20, 2022

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

pllim mentioned this pull request May 24, 2022

BUG: Metadata viewer to ignore invalid FITS header #1342

Merged

9 tasks

pllim changed the title ~~Hotfixes for Ori and Cami demo~~ Cubeviz cube fitting: Fix fitted cube all zeroes, allow MP bypass May 24, 2022

pllim added the cubeviz label May 24, 2022

BUG: Model fitting window must be Quantity

d55fb5e

DEV: Allow multiprocessing bypass when n_cpu is 1

pllim force-pushed the cubeviz-hotfixes branch from 3637fbc to d55fb5e Compare May 24, 2022 16:49

pllim commented May 24, 2022

View reviewed changes

pllim marked this pull request as ready for review May 24, 2022 17:03

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino and kecnry as code owners May 24, 2022 17:03

pllim requested a review from orifox May 24, 2022 17:04

rosteen approved these changes May 24, 2022

View reviewed changes

pllim added the Ready for final review label May 24, 2022

kecnry approved these changes May 25, 2022

View reviewed changes

rosteen merged commit cf1caa0 into spacetelescope:main May 25, 2022

pllim deleted the cubeviz-hotfixes branch May 25, 2022 14:13

pllim mentioned this pull request May 25, 2022

DOC: API doc improvements #1346

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cubeviz cube fitting: Fix fitted cube all zeroes, allow MP bypass #1333

Cubeviz cube fitting: Fix fitted cube all zeroes, allow MP bypass #1333

pllim commented May 20, 2022 •

edited

Loading

codecov bot commented May 20, 2022 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

pllim left a comment

pllim May 24, 2022

pllim May 24, 2022

rosteen May 24, 2022

kecnry May 25, 2022

rosteen May 25, 2022

pllim May 25, 2022

kecnry May 25, 2022

pllim May 25, 2022

pllim May 25, 2022

rosteen left a comment

kecnry left a comment

rosteen commented May 25, 2022

		assert fitted_spectrum.flux.unit == u.Jy
		assert not np.all(fitted_spectrum.flux.value == 0)

	n_cpu : `None` or int
	This is only used for spectral cube fitting.
	Number of cores to use for multiprocessing.
	Using all the cores at once is not recommended.
	If `None`, it will use max cores minus one.
	Set this to 1 for debugging.

Cubeviz cube fitting: Fix fitted cube all zeroes, allow MP bypass #1333

Cubeviz cube fitting: Fix fitted cube all zeroes, allow MP bypass #1333

Conversation

pllim commented May 20, 2022 • edited Loading

Description

Checklist for package maintainer(s)

codecov bot commented May 20, 2022 • edited Loading

Codecov Report

This comment was marked as resolved.

This comment was marked as resolved.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

rosteen commented May 25, 2022

pllim commented May 20, 2022 •

edited

Loading

codecov bot commented May 20, 2022 •

edited

Loading