Multislit wcsinfo#10464
Conversation
|
tests with stdatamodels main: https://github.com/spacetelescope/RegressionTests/actions/runs/24580487414 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10464 +/- ##
=======================================
Coverage 86.16% 86.16%
=======================================
Files 372 372
Lines 40092 40102 +10
=======================================
+ Hits 34544 34555 +11
+ Misses 5548 5547 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
melanieclarke
left a comment
There was a problem hiding this comment.
Looks good! I agree that the minor changes to products here should all be good changes.
|
https://github.com/spacetelescope/RegressionTests/actions/runs/24726585245 clean run for okify |
|
I find this solution a bit less intuitive than assigning the S_REGION to the primary metadata - if we assign it to one slit, it should probably apply to all of the slits? I think the solution in JP-4100 was even less elegant but prompted by a need to store the metadata in spectral products. All that said, if this approach is meant to avoid stdatamodels#712 due to complications it causes, then this works for me. |
|
@tapastro yes avoiding spacetelescope/stdatamodels#712 was the intent here. Should we open a follow-up issue to consider more possible ways of handling this? |
tapastro
left a comment
There was a problem hiding this comment.
With the new ticket filed and this PR matching existing patterns, I'm going to approve. We can talk about modifications to this approach on that ticket.
This PR addresses some instances where
wcsinfowas taken from the top-levelmodel.meta.wcsinfoof aMultiSlitModeldespite that attribute being schema-undefined. This PR makes it so thewcsinfoof individual slits is used instead.Based on discussion in https://jira.stsci.edu/browse/JP-4100 the addition of the parent grism S_REGION is an improvement - it seems like Tyler and Jo Taylor expected it would be there.
Prerequisite for spacetelescope/stdatamodels#710 but they don't need to be merged at the same time, this PR can go in first.
Tasks
Build 12.0(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)changes/<PR#>.breaking.rstnews fragmentdocs/pageokify_regteststo update the truth files