-
Notifications
You must be signed in to change notification settings - Fork 21
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
RCAL-950: CRDS Keywords & Misc #423
RCAL-950: CRDS Keywords & Misc #423
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
==========================================
- Coverage 97.56% 97.47% -0.10%
==========================================
Files 30 36 +6
Lines 2788 3439 +651
==========================================
+ Hits 2720 3352 +632
- Misses 68 87 +19 ☔ View full report in Codecov by Sentry. |
…dels into RCAL-950_CRDSKeywordsMisc
…we/roman_datamodels into RCAL-950_CRDSKeywordsMisc
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.
Looks good to me.
@@ -394,7 +394,7 @@ def mk_ref_file(**kwargs): | |||
ref_file["inverse_linearity"] = kwargs.get("inverse_linearity", "N/A") | |||
ref_file["photom"] = kwargs.get("photom", "N/A") | |||
ref_file["area"] = kwargs.get("area", "N/A") | |||
ref_file["crds"] = kwargs.get("crds", {"sw_version": "12.3.1", "context_used": "roman_0815.pmap"}) | |||
ref_file["crds"] = kwargs.get("crds", {"version": "12.3.1", "context": "roman_0815.pmap"}) |
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.
Just out of curiosity, is there a way to fetch version
and context
from CRDS so that we don't have to hardcode it here?
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 just found out about stpipe.crds_client
's get_svn_version()
and get_context_used()
. Is there a reason for not using them?
@@ -234,7 +234,7 @@ def mk_distortion(*, filepath=None, **kwargs): | |||
return save_node(distortionref, filepath=filepath) | |||
|
|||
|
|||
def mk_epsf(*, shape=(3, 9, 361, 361), filepath=None, **kwargs): | |||
def mk_epsf(*, shape=(3, 6, 9, 361, 361), filepath=None, **kwargs): |
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.
Maybe we should set a global var with the default shape to prevent repetition?
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, thanks. @mairanteodoro , in the maker utils, we don't necessarily have CRDS installed, etc., so we don't want to use that here. But definitely in romancal.
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.
Looks good.
Resolves RCAL-950
Closes #1507
This PR addresses updates for Build 17 release of the RAD software package.
Tasks
roman_datamodels
tests.docs/
page.no-changelog-entry-needed
.)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types).romancal
regression test (https://github.com/spacetelescope/RegressionTests/actions/workflows/romancal.yml) with this branch installed ("git+https://github.com/<fork>/rad@<branch>"
).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