-
Notifications
You must be signed in to change notification settings - Fork 25
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
JP-3588: Implement new reference file datamodel for PASTASOSS transformations #320
Conversation
🍝 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
+ Coverage 66.41% 66.44% +0.03%
==========================================
Files 101 102 +1
Lines 5434 5439 +5
==========================================
+ Hits 3609 3614 +5
Misses 1825 1825 ☔ View full report in Codecov by Sentry. |
src/stdatamodels/jwst/datamodels/schemas/soss_tracemodel.schema.yaml
Outdated
Show resolved
Hide resolved
src/stdatamodels/jwst/datamodels/schemas/soss_tracemodel.schema.yaml
Outdated
Show resolved
Hide resolved
src/stdatamodels/jwst/datamodels/schemas/soss_tracemodel.schema.yaml
Outdated
Show resolved
Hide resolved
The suggestions I added don't yet account for the Would you add a unit test that makes an instance of the new model with expected junk data? That would help me understand what should go in those spots. |
a14a1e3
to
c6da3a4
Compare
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.
There one item in the schema that causes the following to fail (which I think is worth adding as a generic test-all-model-schemas test):
import asdf
import stdatamodels.jwst.datamodels as dm
s = dm.PastasossModel()
asdf.schema.check_schema(s.schema)
Otherwise looks good to me.
Note that by default all ndim
validation is disabled due to validate_arrays being default False
. I have a hunch this is off as an attempt to avoid data loading or something but it is suprising to me that this disables ndim validation.
src/stdatamodels/jwst/datamodels/schemas/pastasossmodel.schema.yaml
Outdated
Show resolved
Hide resolved
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.
LGTM!
38f7714
to
2fd5d17
Compare
30410f5
to
428a9c3
Compare
428a9c3
to
f78fb11
Compare
Partially resolves JP-3588
This PR adds a new reference file datamodel for the PASTASOSS algorithm, replacing the existing ATOCA transformation algorithm. This effectively deprecates the spectrace and wavemap datamodels - TBD on how best to remove them from here and/or CRDS.
Checklist
CHANGES.rst
(either inBug Fixes
orChanges to API
)