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

OIFITS compatible schema and AmiOIModel #174

Merged
merged 8 commits into from
Jun 28, 2023

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Jun 22, 2023

Opening this as a draft to trigger the CI to see how the jwst downstream handles the new model (in part to test: spacetelescope/jwst#7607)
Resolves JP-1713

Adds a schema oifits.schema.yaml to create OIFITS compatible files to be used for a new AmiOIModel.

The added test generates a file that is compatible with OIFITS as tested with oifits-check from oifitslib which 'generally' agrees with the online OIFITS checker from JMMC.

100% OIFITS compliance testing is not be possible with the schema (as valid values for some items depend on values for other items, a dependency that cannot be checked with the schema). Additionally, the schema is constructed based off of the example files (which included OI_VIS OI_VIS2 and OI_T3 tables) and allow for one of each of those tables (and requires that at least one be defined).

There was one keyword 'conflict' with the JWST core schema and OIFITS. Both use the DATE-OBS keyword. JWST appears to use this to store a date whereas OIFITS mentions a date and time. However the wording in OIFITS does not specifically require a date time and the OIFITS checkers seem ok with a date. To avoid complications with existing JWST steps this keyword is being left as a date.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@braingram braingram changed the title Oifits OIFITS compatible schema and AmiOImodel Jun 22, 2023
@braingram braingram changed the title OIFITS compatible schema and AmiOImodel OIFITS compatible schema and AmiOIModel Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch coverage: 97.43% and project coverage change: +0.27 🎉

Comparison is base (4f8894a) 63.69% compared to head (d2ed1d3) 63.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   63.69%   63.96%   +0.27%     
==========================================
  Files         100      101       +1     
  Lines        5517     5556      +39     
==========================================
+ Hits         3514     3554      +40     
+ Misses       2003     2002       -1     
Impacted Files Coverage Δ
src/stdatamodels/jwst/datamodels/amioi.py 97.36% <97.36%> (ø)
src/stdatamodels/jwst/datamodels/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@braingram braingram marked this pull request as ready for review June 23, 2023 14:27
@braingram braingram requested a review from a team as a code owner June 23, 2023 14:27
@braingram
Copy link
Collaborator Author

@braingram braingram requested a review from hbushouse June 23, 2023 15:06
@braingram
Copy link
Collaborator Author

Regression tests showed 23 failures in jwst.ramp_fitting.tests @hbushouse are these expected for other reasons?

@hbushouse
Copy link
Contributor

Regression tests showed 23 failures in jwst.ramp_fitting.tests @hbushouse are these expected for other reasons?

Yes, it's because some ramp_fitting updates were recently merged into stcal/main, but updates to the unit tests sitting over in jwst/ramp_fitting/tests/ have not yet been merged, so those unit tests are producing expected differences.

Copy link
Contributor

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. I guess we'll need to confer with the NIRISS AMI folks as to whether it's OK to proceed with only having a date value in the DATE-OBS keyword.

@braingram
Copy link
Collaborator Author

Looks good to me overall. I guess we'll need to confer with the NIRISS AMI folks as to whether it's OK to proceed with only having a date value in the DATE-OBS keyword.

Thanks for the review!

Based off of spacetelescope/jwst#5351 (comment) merging seems like a reasonable next step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants