-
Notifications
You must be signed in to change notification settings - Fork 73
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
Improve Imviz parser for Roman-like ASDF files #2351
Conversation
a81f932
to
49c95a5
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2351 +/- ##
==========================================
+ Coverage 90.66% 90.74% +0.08%
==========================================
Files 152 153 +1
Lines 17434 17470 +36
==========================================
+ Hits 15807 15854 +47
+ Misses 1627 1616 -11
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try-except isn't pretty but good enough for Roman.
Since this touches Roman, I also enabled the Roman cron jobs here. Let's see... 🤞 |
Is the Roman job failure related? I cannot tell. https://github.com/spacetelescope/jdaviz/actions/runs/5811566248/job/15755086033?pr=2351 |
@pllim that's likely from the latest release of |
Bumping the To avoid this problem occurring indefinitely into the future, I've made a pytest fixture to generate this test data for tests where it's needed in c44067e. |
jdaviz/conftest.py
Outdated
@@ -290,6 +292,12 @@ def mos_image(): | |||
return CCDData(data, wcs=wcs, unit='Jy', meta=header) | |||
|
|||
|
|||
@pytest.fixture | |||
@pytest.mark.skipif(not HAS_ROMAN_DATAMODELS, reason="roman_datamodels is not installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting a skipif in a fixture is too confusing. If you want a test to skip, it is more explicit to put skipif on the test function.
If create_wfi_image_model
needs Roman datamodels, then maybe have this return None if that package isn't installed with a if-else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternately, you can move all the stuff that needs Roman data models into a special test module and use pytest.importorskip
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of 8a12ef8, we do both. There's a roman_datamodels
-specific test in jdaviz/configs/imviz/tests/test_parser_roman.py
that uses importorskip
, and now the fixture returns None unless roman_datamodels
is available.
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
8a12ef8
to
33cd9b0
Compare
Diff LGTM now but I want to see the Roman cron job status before approving. Thanks! |
…1-on-v3.6.x Backport PR #2351 on branch v3.6.x (Improve Imviz parser for Roman-like ASDF files)
* Improve Roman-like ASDF support * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Parser fix after Pey Lian's revision * Update CHANGES.rst Co-authored-by: P. L. Lim <[email protected]> * bump rdm pin * removing static test Roman datamodel, generating it in a pytest fixture instead * removing skipif on the romandep fixture --------- Co-authored-by: P. L. Lim <[email protected]>
Description
There are lots of "Roman-like" ASDF files that do not pass validation by
roman_datamodels
, but currently onmain
, attempting to parse a file that cannot be fully parsed byroman_datamodels
raises an error.A minimal "Roman-like" image in an ASDF file could be constructed with:
If the above ASDF file is passed to
roman_datamodels
, an error is raised, even though it's pretty clear that we could parse thedata
node as an image:This PR allows the parser to use
asdf
to look for the usual Roman tree structure in files ending in".asdf"
, even whenroman_datamodels
can't parse the file. This allows non-standard Roman-like data products to be loaded.I say "Roman-like" because this parser is still looking for a top-level node called
"roman"
, which is standard in Roman data products. If that node isn't there, this parser will raise an error. If we need in the future, we can revise this parser logic to accept the preferred top-level node as a kwarg.Change log entry
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, maintainershould 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.
trivial
label.