Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moment map continuum subtraction #2587

Merged
merged 10 commits into from
Dec 12, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Dec 5, 2023

Description

This pull request implements support for continuum subtraction within the moment map plugin in cubeviz. It does so by refactoring code in line-analysis into a reusable component, showing the same input options and visualization (but with additional descriptive text to explain that the continuum is re-computed per-spaxel when computing the moment map).

Screen.Recording.2023-12-05.at.2.00.26.PM.mov

This PR also includes a commit (since reverted but left in the PR diff in case we want to revisit it) which would enable exporting the continuum as a cube back into the app.

TODO:

  • proper deprecation of line_analysis.width -> line_analysis.continuum_width
  • investigate need to handle orig_spec
  • consider moving mark visibility logic to mixin

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)?

@kecnry kecnry added the cubeviz label Dec 5, 2023
@kecnry kecnry added this to the 3.9 milestone Dec 5, 2023
@kecnry kecnry force-pushed the mm-continuum-subtraction branch 2 times, most recently from b8e09f0 to 2a312c6 Compare December 6, 2023 14:39
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

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

Comparison is base (bd312bd) 91.51% compared to head (9b6350f) 91.53%.
Report is 11 commits behind head on main.

❗ Current head 9b6350f differs from pull request most recent head 4a38531. Consider uploading reports for the commit 4a38531 to get more accurate results

Files Patch % Lines
jdaviz/core/template_mixin.py 92.56% 9 Missing ⚠️
...plugins/spectral_extraction/spectral_extraction.py 86.48% 5 Missing ⚠️
...daviz/configs/default/plugins/collapse/collapse.py 86.48% 5 Missing ⚠️
...configs/cubeviz/plugins/moment_maps/moment_maps.py 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2587      +/-   ##
==========================================
+ Coverage   91.51%   91.53%   +0.01%     
==========================================
  Files         161      161              
  Lines       19596    19804     +208     
==========================================
+ Hits        17934    18128     +194     
- Misses       1662     1676      +14     

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

@kecnry kecnry force-pushed the mm-continuum-subtraction branch from 07082e0 to 3460fde Compare December 7, 2023 15:04
@kecnry kecnry force-pushed the mm-continuum-subtraction branch from 91caa3a to 9b6350f Compare December 7, 2023 15:18
@kecnry kecnry marked this pull request as ready for review December 7, 2023 15:32
@javerbukh
Copy link
Contributor

Trigger traceback with the following steps:

  1. Cubeviz example notebook
  2. Create spectral subset
  3. Go to moment map plugin
  4. Spectral region is subset 1
  5. Continuum surrounding
  6. Moment 2 and wavelength
  7. Plot in uncertainty viewer
  8. Press calculate

Here is the traceback:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
File ~\anaconda3\envs\test-mm-sub\lib\site-packages\ipyvue\VueTemplateWidget.py:60, in Events._handle_event(self, _, content, buffers)
     58     getattr(self, "vue_" + event)(data, buffers)
     59 else:
---> 60     getattr(self, "vue_" + event)(data)

File ~\Documents\STScI\jdaviz\jdaviz\configs\cubeviz\plugins\moment_maps\moment_maps.py:228, in MomentMap.vue_calculate_moment(self, *args)
    227 def vue_calculate_moment(self, *args):
--> 228     self.calculate_moment(add_data=True)

File ~\Documents\STScI\jdaviz\jdaviz\core\template_mixin.py:294, in with_spinner.<locals>.decorator.<locals>.wrapper(self, *args, **kwargs)
    292 setattr(self, spinner_traitlet, True)
    293 try:
--> 294     ret_ = meth(self, *args, **kwargs)
    295 except Exception:
    296     setattr(self, spinner_traitlet, False)

File ~\Documents\STScI\jdaviz\jdaviz\configs\cubeviz\plugins\moment_maps\moment_maps.py:177, in MomentMap.calculate_moment(self, add_data)
    175         cube = self.dataset.selected_obj
    176 else:
--> 177     _, _, cube = self._get_continuum(self.dataset,
    178                                      'per-pixel',
    179                                      self.spectral_subset,
    180                                      update_marks=False)
    182 # slice out desired region
    183 # TODO: should we add a warning for a composite spectral subset?
    184 spec_min, spec_max = self.spectral_subset.selected_min_max(cube)

File ~\Documents\STScI\jdaviz\jdaviz\core\template_mixin.py:2087, in SpectralContinuumMixin._get_continuum(self, dataset, spatial_subset, spectral_subset, update_marks)
   2083     sr = self.app.get_subsets(spectral_subset.selected,
   2084                               simplify_spectral=True,
   2085                               use_display_units=True)
   2086     spectrum = extract_region(full_spectrum, sr, return_single_spectrum=True)
-> 2087     sr_lower = np.nanmin(spectrum.spectral_axis[spectrum.spectral_axis.value >= sr.lower.value])  # noqa
   2088     sr_upper = np.nanmax(spectrum.spectral_axis[spectrum.spectral_axis.value <= sr.upper.value])  # noqa
   2090 if self.continuum_subset_selected == 'None':

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\astropy\units\quantity.py:1838, in Quantity.__array_function__(self, function, types, args, kwargs)
   1825 # A function should be in one of the following sets or dicts:
   1826 # 1. SUBCLASS_SAFE_FUNCTIONS (set), if the numpy implementation
   1827 #    supports Quantity; we pass on to ndarray.__array_function__.
   (...)
   1835 # function is in none of the above, we simply call the numpy
   1836 # implementation.
   1837 if function in SUBCLASS_SAFE_FUNCTIONS:
-> 1838     return super().__array_function__(function, types, args, kwargs)
   1840 elif function in FUNCTION_HELPERS:
   1841     function_helper = FUNCTION_HELPERS[function]

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\numpy\lib\nanfunctions.py:350, in nanmin(a, axis, out, keepdims, initial, where)
    347 else:
    348     # Slow, but safe for subclasses of ndarray
    349     a, mask = _replace_nan(a, +np.inf)
--> 350     res = np.amin(a, axis=axis, out=out, **kwargs)
    351     if mask is None:
    352         return res

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\astropy\units\quantity.py:1838, in Quantity.__array_function__(self, function, types, args, kwargs)
   1825 # A function should be in one of the following sets or dicts:
   1826 # 1. SUBCLASS_SAFE_FUNCTIONS (set), if the numpy implementation
   1827 #    supports Quantity; we pass on to ndarray.__array_function__.
   (...)
   1835 # function is in none of the above, we simply call the numpy
   1836 # implementation.
   1837 if function in SUBCLASS_SAFE_FUNCTIONS:
-> 1838     return super().__array_function__(function, types, args, kwargs)
   1840 elif function in FUNCTION_HELPERS:
   1841     function_helper = FUNCTION_HELPERS[function]

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\numpy\core\fromnumeric.py:2970, in amin(a, axis, out, keepdims, initial, where)
   2957 @array_function_dispatch(_min_dispatcher)
   2958 def amin(a, axis=None, out=None, keepdims=np._NoValue, initial=np._NoValue,
   2959          where=np._NoValue):
   2960     """
   2961     Return the minimum of an array or minimum along an axis.
   2962 
   (...)
   2968     ndarray.min : equivalent method
   2969     """
-> 2970     return _wrapreduction(a, np.minimum, 'min', axis, None, out,
   2971                           keepdims=keepdims, initial=initial, where=where)

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\numpy\core\fromnumeric.py:86, in _wrapreduction(obj, ufunc, method, axis, dtype, out, **kwargs)
     84             return reduction(axis=axis, dtype=dtype, out=out, **passkwargs)
     85         else:
---> 86             return reduction(axis=axis, out=out, **passkwargs)
     88 return ufunc.reduce(obj, axis, dtype, out, **passkwargs)

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\numpy\core\_methods.py:45, in _amin(a, axis, out, keepdims, initial, where)
     43 def _amin(a, axis=None, out=None, keepdims=False,
     44           initial=_NoValue, where=True):
---> 45     return umr_minimum(a, axis, None, out, keepdims, initial, where)

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\astropy\coordinates\spectral_quantity.py:87, in SpectralQuantity.__array_ufunc__(self, function, method, *inputs, **kwargs)
     85 def __array_ufunc__(self, function, method, *inputs, **kwargs):
     86     # We always return Quantity except in a few specific cases
---> 87     result = super().__array_ufunc__(function, method, *inputs, **kwargs)
     88     if (
     89         (
     90             function is np.multiply
   (...)
     98         )
     99     ):
    100         result = result.view(self.__class__)

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\astropy\units\quantity.py:691, in Quantity.__array_ufunc__(self, function, method, *inputs, **kwargs)
    689     return NotImplemented
    690 else:
--> 691     raise e

File ~\anaconda3\envs\test-mm-sub\lib\site-packages\astropy\units\quantity.py:666, in Quantity.__array_ufunc__(self, function, method, *inputs, **kwargs)
    663     arrays.append(converter(input_) if converter else input_)
    665 # Call our superclass's __array_ufunc__
--> 666 result = super().__array_ufunc__(function, method, *arrays, **kwargs)
    667 # If unit is None, a plain array is expected (e.g., comparisons), which
    668 # means we're done.
    669 # We're also done if the result was None (for method 'at') or
    670 # NotImplemented, which can happen if other inputs/outputs override
    671 # __array_ufunc__; hopefully, they can then deal with us.
    672 if unit is None or result is None or result is NotImplemented:

ValueError: zero-size array to reduction operation minimum which has no identity

@kecnry
Copy link
Member Author

kecnry commented Dec 7, 2023

What is your spectral subset? Is it possible it doesn't encompass any data (in which case we should check for that - but probably somewhere more general).

@javerbukh
Copy link
Contributor

Is it possible for a spectral subset in cubeviz to not encompass any data? I made it in the middle of the collapsed flux spectrum.

@pllim
Copy link
Contributor

pllim commented Dec 8, 2023

Is it possible for a spectral subset in cubeviz to not encompass any data?

If you highlight a region where there is a gap, it is technically possible. But usually people would only highlight a region of interest like emission/absorption lines, right?

@javerbukh
Copy link
Contributor

Still seeing this in a new environment while changing where I make the spectral subset selection. Here is my environment:

Windows-10-10.0.19045-SP0
Python 3.10.13 | packaged by Anaconda, Inc. | (main, Sep 11 2023, 13:24:38) [MSC v.1916 64 bit (AMD64)]
Numpy 1.26.2
astropy 6.0.0
matplotlib 3.8.2
scipy 1.11.4
scikit-image 0.22.0
asdf 3.0.1
stdatamodels 1.8.4
gwcs 0.20.0
regions 0.8
specutils 1.12.0
specreduce 1.3.0
photutils 1.10.0
astroquery 0.4.6
pyyaml 6.0.1
asteval 0.9.31
idna 3.6
traitlets 5.14.0
bqplot 0.12.42
bqplot-image-gl 1.4.11
glue-core 1.17.1
glue-jupyter 0.20.0
glue-astronomy 0.10.0
echo 0.8.0
ipyvue 1.10.1
ipyvuetify 1.8.10
ipysplitpanes 0.2.0
ipygoldenlayout 0.4.0
ipypopout 1.2.0
Jinja2 3.1.2
voila 0.4.3
vispy 0.14.1
sidecar 0.7.0
Jdaviz 3.8.1.dev19+g9b6350ff.d20231211

@kecnry kecnry requested a review from gibsongreen as a code owner December 11, 2023 18:54
Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin works well and everything behaves as intended, although I'll let someone with more knowledge of continuum subtraction check the results of the tests. Nice work!

Copy link
Contributor

@gibsongreen gibsongreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed the video workflow and code changes, awesome stuff; it's got the green light from me!

@kecnry
Copy link
Member Author

kecnry commented Dec 11, 2023

the green light

ha! 🤣 🟢

@kecnry kecnry merged commit c88afa2 into spacetelescope:main Dec 12, 2023
16 of 17 checks passed
@kecnry kecnry deleted the mm-continuum-subtraction branch December 12, 2023 13:55
@pllim pllim mentioned this pull request Jan 31, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants