glue-viz / glue-astronomy

Plugin to add astronomy-specific functionality to glue
https://glue-astronomy.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

BUG: Do not crash when masking fails #39

Closed pllim closed 2 years ago

pllim commented 2 years ago

Description

In Cubeviz workflow, when user creates subset on the collapsed image (2D) and then select that subset for model fitting plugin, it tries to grab a 2D mask from a 1D spectrum, which crashes spectacularly. User sees a traceback albeit successful fitting. (Success here means something shows; I have no idea if the fit is actually correct or not, but that is out of scope here.)

This patch prevent get_mask from crashing when this use case is encountered.

Part of https://jira.stsci.edu/browse/JDAT-1671

p.s. Not sure how much this conflicts with #36

codecov[bot] commented 2 years ago

Codecov Report

Merging #39 (99cb9cf) into master (4c5e6e7) will decrease coverage by 0.18%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
- Coverage   96.40%   96.22%   -0.19%     
==========================================
  Files          15       15              
  Lines        1002     1006       +4     
==========================================
+ Hits          966      968       +2     
- Misses         36       38       +2     
Impacted Files Coverage Δ
glue_astronomy/translators/spectrum1d.py 94.73% <66.66%> (-2.49%) :arrow_down:

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 4c5e6e7...99cb9cf. Read the comment docs.

astrofrog commented 2 years ago

I'm on board with something like this but I wonder if the mask should actually be all elements, as if it was an empty subset? (a mask of None means it will return all data values)

astrofrog commented 2 years ago

On second thoughts I think raising an error here is the right thing to do, but we should raise a more understandable error - for example IncompatibleSubsetState (I just made that up, it is not an existing exception).

pllim commented 2 years ago

I think raising an error here is the right thing to do

So, are you saying this PR cannot be accepted? In that case, I'll have to find an alternate solution to the crashing.

astrofrog commented 2 years ago

Yes, what I mean is that raising an error is the right thing to do and it is up to jdaviz to catch cases where this is called and an exception is raised.

astrofrog commented 2 years ago

The commit I added to https://github.com/spacetelescope/jdaviz/pull/762 shows how one might catch the exception on the jdaviz side - I'm not sure if I got all instances of it, I was just trying to stop things from crashing in that PR. But fundamentally if you ask to translate an object where the subset can't be computed, it's correct that we should raise an error here.

pllim commented 2 years ago

Superseded by spacetelescope/jdaviz#806