-
Notifications
You must be signed in to change notification settings - Fork 33
Update the reference file schemas to use explicit entries #712
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
base: main
Are you sure you want to change the base?
Update the reference file schemas to use explicit entries #712
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 97.76% 89.19% -8.57%
==========================================
Files 8 14 +6
Lines 759 1037 +278
==========================================
+ Hits 742 925 +183
- Misses 17 112 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f2cd86d to
97cac2f
Compare
97cac2f to
e9fc715
Compare
| $ref: asdf://stsci.edu/datamodels/roman/schemas/reference_files/wfi_img_photom-1.3.0#/definitions/empty_phot_table_entry | ||
| NOT_CONFIGURED: | ||
| $ref: asdf://stsci.edu/datamodels/roman/schemas/reference_files/wfi_img_photom-1.3.0#/definitions/empty_phot_table_entry | ||
| required: |
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.
Do we have confirmation from RTB that every new reference file will contain all of these items (same for the other new required added in this PR)?
| data: | ||
| type: object | ||
| patternProperties: | ||
| "^(F062|F087|F106|F129|F146|F158|F184|F213|GRISM|PRISM|DARK)$": |
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.
I find this format easier to follow.
If there is a want to make some of these required we can update the builder in rdm to handle this as there's nothing else preventing us from adding required here (it's supported by asdf and jsonschema).
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.
I do not. But that is my opinion.
Currently all the published reference files have all of these in place so I see no reason not to require them.
|
I'll defer to you about what structure makes the most sense. Re what should be required and what shouldn't...
I think we could treat some of these things as "it's nice to have these for some kind of tests that things exist for all of the optical elements", but I don't think we should requires these scientifically. Requiring grism / prism / grism_0 in phot could serve as a check that RTB continues to populate these in the future; the community could come to depend on them. But we don't use them and the pipeline should run without them. |
e9fc715 to
bbf3348
Compare
Closes #709
As noted in the closed issue it makes more sense for these schemas to explicitly define entries for each of the optical elements via a definition rather than using a
patternPropertiesargument. This PR is based off #630 to incorporate those changes cleanly into this.RDM PR: spacetelescope/roman_datamodels#577
Tasks
radtests.docs/page.no-changelog-entry-needed.)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types).romancalregression test (https://github.com/spacetelescope/RegressionTests/actions/workflows/romancal.yml) with this branch installed ("git+https://github.com/<fork>/rad@<branch>").roman_datamodelsutilities and tests.News fragment change types:
changes/<PR#>.feature.rst: new featurechanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.doc.rst: documentation changechanges/<PR#>.removal.rst: deprecation or removal of public APIchanges/<PR#>.misc.rst: infrastructure or miscellaneous change