Add test that triggers handle_display_units bug #2910

javerbukh · 2024-06-05T12:58:52Z

Description

This pull request is to address a bug that exists on main but was made more apparent by #2873 . Currently there is just a couple lines in the unit conversion test that triggers this in CI, but the traceback can also be seen by running the following lines in the cubeviz example notebook after data has been loaded:

uc = cubeviz.plugins['Unit Conversion']._obj
la = cubeviz.plugins['Line Analysis']
la.open_in_tray()
uc.show_translator = True
uc.flux_or_sb.selected = 'Flux'

and in a new cell after the Line Analysis plugin has opened and the continuum is visible:
la.get_results()

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. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2024-06-05T14:26:59Z

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.77%. Comparing base (1733f43) to head (87b1a8a).
Report is 7 commits behind head on main.

Files	Patch %	Lines
jdaviz/core/helpers.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2910      +/-   ##
==========================================
+ Coverage   88.76%   88.77%   +0.01%     
==========================================
  Files         111      111              
  Lines       17178    17194      +16     
==========================================
+ Hits        15248    15264      +16     
  Misses       1930     1930

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

gibsongreen

This looks good to me, will be very nice to have the changes at the helper level. Great work!

bmorris3 · 2024-06-14T18:32:53Z

jdaviz/app.py

@@ -109,72 +109,76 @@ def to_unit(self, data, cid, values, original_units, target_units):
                spec = data.get_object(cls=Spectrum1D)

            except RuntimeError:
-                eqv = []
+                data = data.get_object(cls=NDDataArray)
+                spec = Spectrum1D(flux=data.data * u.Unit(original_units))


Should we pass along the attributes for spectral axis, meta, or WCS, too? There are places within the methods below that call Spectrum1D's spectral_axis and meta, so we might need to make sure those get passed in here too.

What value should I pass for the spectral_axis parameter? Otherwise yes that makes sense.

Good question. We could pull it out of the wcs by getting out the spectral component of the coordinates, and evaluate it on the integer pixel grid.

BUT I'm pretty sure if you give Spectrum1D the WCS without separately specifying spectral axis, it will do that parsing for you.

Adding the wcs did not work for me, would you be ok with me creating a follow-up ticket for roundtripping all relevant information? I would like to get this in soon so I can finish with #2873 and have this available for the aperture photometry work.

Sure thing 💯

jdaviz/app.py

javerbukh · 2024-06-18T12:57:49Z

jdaviz/configs/cubeviz/plugins/moment_maps/tests/test_moment_maps.py

+    # TODO: Failing because of dev version of upstream dependency, figure
+    #  out which one
+    # assert mm.calculate_moment()
+
+    # TODO: This test should pass once continuum subtraction works with
+    #  flux to surface brightness conversion
+    # mm.continuum.selected = 'Surrounding'
+    #
+    # assert mm.calculate_moment()


These tests will need to be uncommented when we make the unit conversion translator available to users. Otherwise, this PR is ready for a re-review @bmorris3 .

bmorris3

This looks great. Please add a changelog entry and you're good to go.

Add test that triggers handle_display_units bug

b0a6bf6

javerbukh requested review from rosteen, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners June 5, 2024 12:58

github-actions bot added specviz plugin Label for plugins common to multiple configurations labels Jun 5, 2024

javerbukh added this to the 4.0 milestone Jun 5, 2024

Pass test with hardcoded values

e82dd7f

javerbukh marked this pull request as draft June 5, 2024 14:02

Pass style checks

9f9a4e3

javerbukh added 6 commits June 6, 2024 08:49

Update handle_display_units

f131164

Fix style

e546bba

Fix import and style issues

fbbbecc

Rename variable

a8d8523

Try to handle 2D objects using Spectrum1D

9d357bc

Remove commented code

040ffc8

javerbukh marked this pull request as ready for review June 11, 2024 14:43

gibsongreen approved these changes Jun 14, 2024

View reviewed changes

bmorris3 reviewed Jun 14, 2024

View reviewed changes

javerbukh added 5 commits June 14, 2024 16:10

Fix moment map with translator on, leave comments for continuum

7265a4c

Fix code style

5fddf6e

Remove wcs from NDDataArray in to_unit

b573552

Revert review changes

f6e8bec

Comment out test until unit conversion translator is active

ba1a2ec

javerbukh commented Jun 18, 2024

View reviewed changes

javerbukh added 2 commits June 18, 2024 14:23

Pass wcs when converting to spectrum1d from nddataarray

116ca41

Do not pass wcs

0bcefa7

bmorris3 approved these changes Jun 18, 2024

View reviewed changes

Add change log

87b1a8a

javerbukh merged commit a1d9682 into spacetelescope:main Jun 18, 2024
19 checks passed

This was referenced Jul 2, 2024

Investigate moment map unit conversion #2935

Closed

Investigate moment map commented test #2936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test that triggers handle_display_units bug #2910

Add test that triggers handle_display_units bug #2910

javerbukh commented Jun 5, 2024

codecov bot commented Jun 5, 2024 •

edited

Loading

gibsongreen left a comment

bmorris3 Jun 14, 2024

javerbukh Jun 14, 2024

bmorris3 Jun 18, 2024

bmorris3 Jun 18, 2024

javerbukh Jun 18, 2024

bmorris3 Jun 18, 2024

javerbukh Jun 18, 2024

bmorris3 left a comment

Add test that triggers handle_display_units bug #2910

Add test that triggers handle_display_units bug #2910

Conversation

javerbukh commented Jun 5, 2024

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Jun 5, 2024 • edited Loading

Codecov Report

gibsongreen left a comment

Choose a reason for hiding this comment

bmorris3 Jun 14, 2024

Choose a reason for hiding this comment

javerbukh Jun 14, 2024

Choose a reason for hiding this comment

bmorris3 Jun 18, 2024

Choose a reason for hiding this comment

bmorris3 Jun 18, 2024

Choose a reason for hiding this comment

javerbukh Jun 18, 2024

Choose a reason for hiding this comment

bmorris3 Jun 18, 2024

Choose a reason for hiding this comment

javerbukh Jun 18, 2024

Choose a reason for hiding this comment

bmorris3 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2024 •

edited

Loading