Handle missing minItems.#692
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #692 +/- ##
==========================================
- Coverage 95.21% 95.17% -0.04%
==========================================
Files 27 27
Lines 2861 2861
==========================================
- Hits 2724 2723 -1
- Misses 137 138 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # No minimum length is imposed by the schema, so keep however many | ||
| # defaults were provided (as the scalar builders do) instead of | ||
| # discarding them. Without a default this yields an empty array. | ||
| min_items = len(defaults) |
There was a problem hiding this comment.
The general strategy here is that create_minimal only uses defaults if the item is required.
read_pattern is required but the items/structure is undefined in the schema:
https://github.com/spacetelescope/rad/blob/f82c94f91e8afabf3348a2be870790c8f129f6b6/latest/meta/exposure.yaml#L261
So the minimal amount that can be taken from the defaults and is valid is an empty read pattern.
I'm ok with the approach in this PR (if the schema has no minItems but a default was provided, use the defaults) given the limited use of lists/arrays in our schemas. An alternative would be to propagate read_pattern in romancal (since I don't see a way for us to define minItems or something similar in the schema).
Given the defaults is copied below (341) and then len checked (344) I think we could replace this line with:
| min_items = len(defaults) | |
| return copy.deepcopy(defaults) |
There was a problem hiding this comment.
The test in spacetelescope/romancal#2390 only checks read_pattern but there are a handful of other related cases the code should be able to handle (image_shape; rotation_matrix; dither_executed_pattern). Do you see a mechanism to propagate these generically in romancal? It seemed more fragile to list out those cases.
I'm happy with replacing the min_items definition with the direct copy; an earlier version took that approach but I decided it was nicer to follow the path here. I don't have a strong preference.
Re "The general strategy here is that create_minimal only uses defaults if the item is required."---this change is intended to fix
https://github.com/spacetelescope/romancal/blob/main/romancal/source_catalog/source_catalog_step.py#L317-L319
where conceptually we want to bring over the old metadata into the new object for relevant fields. In that context, all things being equal, I'd rather also propagate optional fields, and when copying things over, we want to copy all of them. Do you think that instead we should be using something other than create_minimal there?
There was a problem hiding this comment.
If optional fields are also wanted than maybe this should be using create_from_model instead of create_minimal?
I tried this PR with a few other attributes and one thing to note is that now cal_logs are copied from the defaults for create_minimal. I don't think that's an issue (since those should be overwritten when a step finishes).
2 participants
This PR intends to improve the handling of read_pattern and other attributes lacking minItems (c.f. spacetelescope/romancal#2390). Really a Claude PR deserving more scrutiny.
Tasks
roman_datamodelstests.docs/page.no-changelog-entry-needed.)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types).romancalregression 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