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

RAD-502: CRDS Keywords Not Required #506

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

PaulHuwe
Copy link
Collaborator

Resolves RAD-1234

Closes #502

This PR adds CRDS and reference steps required lists.

Tasks

  • Update or add relevant rad tests.
  • Update relevant docstrings and / or docs/ page.
  • Does this PR change any schema files?
    • Schema changes were discussed at RAD Review Board meeting.
  • Does this PR change any API used downstream? (If not, label with no-changelog-entry-needed.)
News fragment change types:
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

@PaulHuwe PaulHuwe marked this pull request as ready for review November 11, 2024 22:24
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.81%. Comparing base (af9de57) to head (5e66e76).
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #506   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files           4        4           
  Lines         215      215           
=======================================
  Hits          206      206           
  Misses          9        9           

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

@WilliamJamieson
Copy link
Collaborator

This looks like a duplicate of #505

@PaulHuwe
Copy link
Collaborator Author

This looks like a duplicate of #505

This is the part that is needed for release, and the #505 stuff is for after the release.

@@ -171,4 +172,5 @@ properties:
datatype: nvarchar(120)
destination: [ScienceRefData.r_refpix, GuideWindow.r_refpix, WFICommon.r_refpix]
flowStyle: block
required: [crds, dark, distortion, mask, flat, gain, readnoise, linearity, photom, area, saturation, refpix]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know whether this breaks existing files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not offhand - given that the other change will break files, I didn't look

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. Let's wait until after standup and I ping Melissa about any other issues she may have identified. But then let's limit this to the minimum change to fix the problem (just making the two crds keywords required, I think?), and then merge unless there are more problems identified.

I think the other required changes are good but but aren't necessary to get things to work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does romancal also need a PR to update where the crds information is assigned?
https://github.com/spacetelescope/romancal/blob/8a04f5a2b604eee12c10a4a2cd7ca96312523b5a/romancal/stpipe/core.py#L90-L91

Copy link
Collaborator Author

@PaulHuwe PaulHuwe Nov 12, 2024

Choose a reason for hiding this comment

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

I think so - we know that files will need to be remade with these changes.

@schlafly
Copy link
Collaborator

Yes, it will.

@PaulHuwe PaulHuwe merged commit f2d7af7 into spacetelescope:main Nov 12, 2024
12 checks passed
@PaulHuwe PaulHuwe deleted the CRDSKeywordsReq branch November 12, 2024 22:40
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.

CRDS Keywords Not Required
4 participants