Consolidate Cubeviz parser conversions #3221

pllim · 2024-10-16T02:16:22Z

Description

This pull request is leverages the following specutils PRs

to simplify Cubeviz parser unit conversion logic. I also refactored it a bit to streamline the flow because things are looking a bit chaotic with different people inserting parts of this and that in the parser.

As a result, I might have also fixed bugs in some uncommon cases that we test for but users do not usually encounter.

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

pllim · 2024-10-16T02:19:19Z

jdaviz/configs/cubeviz/plugins/parsers.py


    meta = standardize_metadata({'_orig_spatial_wcs': None})
    s3d = Spectrum1D(flux=flux, meta=meta)

    # convert data loaded in flux units to a per-square-pixel surface
    # brightness unit (e.g Jy to Jy/pix**2)
-    if not check_if_unit_is_per_solid_angle(s3d.unit):
-        file_obj = convert_spectrum1d_from_flux_to_flux_per_pixel(s3d)


I think assigning file_obj here is actually a bug as file_obj does not get used after this line.

jdaviz/configs/cubeviz/plugins/parsers.py

pllim · 2024-10-16T02:20:17Z

jdaviz/configs/cubeviz/plugins/parsers.py

@@ -580,67 +574,3 @@ def _get_data_type_by_hdu(hdu):
    else:
        data_type = ''
    return data_type
-
-
-def convert_spectrum1d_from_flux_to_flux_per_pixel(spectrum):


This is now unnecessary, as Clare requested.

pllim · 2024-10-16T02:20:57Z

jdaviz/configs/cubeviz/plugins/tests/test_parsers.py

@@ -194,15 +194,15 @@ def test_numpy_cube(cubeviz_helper):
    assert data.label == 'Array'
    assert data.shape == (4, 3, 2)  # x, y, z
    assert isinstance(data.coords, PaddedSpectrumWCS)
-    assert flux.units == 'ct'
+    assert flux.units == 'ct / pix2'


I think this fixes a bug where pure numpy cube does not get that PIX2 assigned like all the other stuff loaded into Cubeviz, right?

Nice catch! I just checked this on main and it wasn't being coerced

pyproject.toml

pllim · 2024-10-16T02:22:04Z

@cshanahan1 , does this look about right?

codecov · 2024-10-16T02:36:36Z

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (56e1ded) to head (51a9a78).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/configs/cubeviz/plugins/parsers.py	92.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3221      +/-   ##
==========================================
- Coverage   88.56%   88.55%   -0.01%     
==========================================
  Files         125      125              
  Lines       18755    18739      -16     
==========================================
- Hits        16610    16595      -15     
+ Misses       2145     2144       -1

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

cshanahan1

looks good- tested it on a bunch of units loaded and it works as intended, no comments. thanks!

gibsongreen · 2024-10-18T18:52:38Z

jdaviz/configs/cubeviz/plugins/parsers.py

@@ -522,15 +521,15 @@ def _parse_ndarray(app, file_obj, data_label=None, data_type=None,
    flux = file_obj

    if not hasattr(flux, 'unit'):
-        flux = flux << u.count
+        flux = flux << (u.count / PIX2)


Can we rename the file_obj parameter to flux in the method signature?

We can but it is out of scope.

gibsongreen

All the parser changes are concise, I tested the different input cubes for the parsers that were updated and all results are as expected, great work!

pllim · 2024-10-18T20:29:50Z

Thanks for the reviews!

Consolidate Cubeviz parser conversions

6241f16

pllim added no-changelog-entry-needed changelog bot directive Affects-dev changelog bot directive labels Oct 16, 2024

pllim added this to the 4.0 milestone Oct 16, 2024

github-actions bot added cubeviz plugin Label for plugins common to multiple configurations labels Oct 16, 2024

pllim commented Oct 16, 2024

View reviewed changes

pllim added 3 commits October 16, 2024 17:35

Update pyproject.toml

6980a02

Ooops typo

40e07d4

Merge branch 'main' into rm-cubeviz-conv-func

51a9a78

rosteen modified the milestones: 4.0, 4.1 Oct 17, 2024

pllim marked this pull request as ready for review October 18, 2024 14:46

pllim requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners October 18, 2024 14:46

cshanahan1 approved these changes Oct 18, 2024

View reviewed changes

gibsongreen reviewed Oct 18, 2024

View reviewed changes

gibsongreen approved these changes Oct 18, 2024

View reviewed changes

pllim merged commit 0ffca43 into spacetelescope:main Oct 18, 2024
20 checks passed

pllim deleted the rm-cubeviz-conv-func branch October 18, 2024 20:29

pllim mentioned this pull request Nov 15, 2024

BUG: Fix Cubeviz parsing DN and DN/s #3298

Merged

9 tasks

rosteen mentioned this pull request Nov 19, 2024

Improve spaxel unit handling for cubes #3307

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate Cubeviz parser conversions #3221

Consolidate Cubeviz parser conversions #3221

pllim commented Oct 16, 2024 •

edited

Loading

pllim Oct 16, 2024

pllim Oct 16, 2024

pllim Oct 16, 2024

gibsongreen Oct 18, 2024

pllim commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

cshanahan1 left a comment

gibsongreen Oct 18, 2024

pllim Oct 18, 2024

gibsongreen left a comment

pllim commented Oct 18, 2024

Consolidate Cubeviz parser conversions #3221

Consolidate Cubeviz parser conversions #3221

Conversation

pllim commented Oct 16, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim Oct 16, 2024

Choose a reason for hiding this comment

pllim Oct 16, 2024

Choose a reason for hiding this comment

pllim Oct 16, 2024

Choose a reason for hiding this comment

gibsongreen Oct 18, 2024

Choose a reason for hiding this comment

pllim commented Oct 16, 2024

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

cshanahan1 left a comment

Choose a reason for hiding this comment

gibsongreen Oct 18, 2024

Choose a reason for hiding this comment

pllim Oct 18, 2024

Choose a reason for hiding this comment

gibsongreen left a comment

Choose a reason for hiding this comment

pllim commented Oct 18, 2024

pllim commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading