spacetelescope / roman_datamodels

Datamodel support for the roman calibration pipeline
https://roman-datamodels.readthedocs.io
Other
7 stars 21 forks source link

only use roman.meta for get_crds_parameters #372

Closed braingram closed 2 weeks ago

braingram commented 2 months ago

Closes #367 Closes #366

This PR modifies DataModel.get_crds_parameters to only include (and only access) items under roman.meta. get_crds_parameters is called twice during the setup of commands like the following strun MyStep mydata.asdf. Once to get the pars file for the MyStep and again to prefetch reference files for mydata.asdf. Each of these calls will:

With this PR only the nodes under roman.meta are accessed and included for get_crds_parameters. This PR does not query crds for the required parameter selectors (parkeys) as:

Querying crds would allow fewer metadata entries to be loaded (which is a very minor improvement that might be offset by the crds API) and would protect against the unlikely case that a non-roman.meta attribute is used as a selector.

This PR also fixes the "include_arrays" filtering for DataModel.to_flat_dict.

Regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/10268370867 show unrelated failures due to background changes.

Checklist

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.77%. Comparing base (087a60d) to head (d29955c). Report is 52 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #372 +/- ## ========================================== + Coverage 97.56% 97.77% +0.21% ========================================== Files 30 36 +6 Lines 2788 3369 +581 ========================================== + Hits 2720 3294 +574 - Misses 68 75 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 2 months ago

I have no opinion about this making it into the current (or next release). If it doesn't make it into this release I'll update the changelog after the release.

schlafly commented 4 weeks ago

Sorry that it's been a while since you filed this.

With this PR only the nodes under roman.meta are accessed and included for get_crds_parameters. This PR does not query crds for the required parameter selectors (parkeys) as:

there doesn't appear to be an API for reliably getting these parkeys even with an API some coordination with stpipe would likely be called for as it has a crds dependency (unlike roman_datamodels)

Just making sure I follow, you're saying that you could ask CRDS to say what it needs, and then send it back exactly that, rather than just sending the entire meta contents minus the arrays. I hear you and agree that we shouldn't need to do that and your PR is good as is.