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

BUG: Recreate JWST MJD-OBS if it is missing #1004

Merged
merged 4 commits into from
Dec 17, 2021

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Dec 14, 2021

Description

This pull request is to recreate missing MJD-OBS for JWST data so it will load into Cubeviz.

Proper GWCS support is not possible currently due to glue-viz/glue-astronomy#59 (the cube would load but it chokes on 1D spectrum part).

With this patch:

Screenshot 2021-12-16 180236

Fixes #690

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? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@pllim pllim added the bug Something isn't working label Dec 14, 2021
@pllim pllim added this to the 2.2 milestone Dec 14, 2021
@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Attention: Patch coverage is 4.54545% with 21 lines in your changes missing coverage. Please review.

Project coverage is 72.45%. Comparing base (f26a213) to head (bb753f7).
Report is 2827 commits behind head on main.

Files Patch % Lines
jdaviz/configs/cubeviz/plugins/parsers.py 4.54% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
- Coverage   72.63%   72.45%   -0.19%     
==========================================
  Files          74       74              
  Lines        5576     5590      +14     
==========================================
  Hits         4050     4050              
- Misses       1526     1540      +14     

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

@pllim pllim marked this pull request as ready for review December 14, 2021 15:35
@pllim pllim requested a review from havok2063 December 14, 2021 15:35
@pllim
Copy link
Contributor Author

pllim commented Dec 14, 2021

The missing coverage is because I don't have permission to include the original offending data file into unit test. I can add one more test here with dummy header but I am not if it is worth it. What do you think?

Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

This looks good and works well!

Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

This looks good to me. When I tested it locally, the ERR and DQ extensions do not load in Cubeviz for me. I remember this problem from before and I thought we fixed it? In any case that is a separate issue than this one.

If we already have the framework for testing against fake files, then I might add some tests for this. But if it's a lot of extra work to create that framework, then I'm ok not having those tests.

@pllim
Copy link
Contributor Author

pllim commented Dec 16, 2021

Re: ERR and DQ -- I think they stopped working after #547 (transition from spectral-cube to specutils). I can see if I can make them load without too much extra code here.

Re: tests -- AFAIK, this is the only thing being tested in the tests for Cubeviz parser. In this patch, I already removed MJD-OBS from it to mimic your problem (though I didn't add in OBSGEO). A more proper way to test is to make fake small test files to ship with the test suite but I am afraid it will be more involved and probably out of scope here.

@pllim
Copy link
Contributor Author

pllim commented Dec 16, 2021

Too late for your test data (i.e., we still need this workaround for now) but Nadia put a patch to astropy at astropy/astropy#12598 . FYI.

@pllim
Copy link
Contributor Author

pllim commented Dec 16, 2021

"latest dev" job failure is unrelated. Looks like something changed upstream:

ERROR: No matching distribution found for asdf-astropy

@pllim
Copy link
Contributor Author

pllim commented Dec 16, 2021

@havok2063 , can you please review again? All the extensions you care about should load now.

Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

Tested again. All looks good to me.

@pllim pllim merged commit aaa2e85 into spacetelescope:main Dec 17, 2021
@pllim pllim deleted the my-fair-mjd branch December 17, 2021 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nirspec ifu data fails to load in cubeviz with MJD errors
3 participants