Skip to content

RAD-216: Remove the use of the tagged scalars take 2#688

Merged
WilliamJamieson merged 4 commits into
spacetelescope:mainfrom
WilliamJamieson:remove_tagged_scalars_v2
Sep 25, 2025
Merged

RAD-216: Remove the use of the tagged scalars take 2#688
WilliamJamieson merged 4 commits into
spacetelescope:mainfrom
WilliamJamieson:remove_tagged_scalars_v2

Conversation

@WilliamJamieson
Copy link
Copy Markdown
Collaborator

@WilliamJamieson WilliamJamieson commented Aug 30, 2025

Resolves RAD-216

Closes #649
Closes #666

This is a second attempt at what #666 was trying to do. This makes no effort to reorganize any schemas nor does it add any new features to RAD.

This makes all the tagged scalars static schemas and removes them from common use. This is based off #687 so that its introduced unit tests could be used to identify all the tags that needed to be moved to static.

Note the changes necessary in RDM and Romancal to make their normal unit tests pass are actually quite small. However, as one can see from the regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/17336765345/job/49224272637. We run into most of the regression tests failing. Looking through the errors they all have the same theme due to type/tag mismatch, I think there are three cases:

  1. The input file still has tagged scalars and so the fact that file_date is now a simple time tag, when that gets copied into the fresh new file validation fails on the final write due to a tag mismatch. This occurs when the model type gets changed by RCAL during the step execution in the regression test.
  2. Similar to 1. the input file still has tagged scalars except this time the model type does not get changed during the step, but one of the tagged scalars gets updated resulting. Since the model does not get changed it stays tagged as the previous datamodel tag but now RDM does not wrap the value into the TaggedScalar so we get a tag mismatch in the opposite direction.
  3. The step runs successfully, and the file validates however the deep diff with the truth fails because there are type differences due to the lack of TaggedScalar wrappers in the new output.

I am pretty sure that if we regenerate all the data files everything should be fine or possibly run create_from_model from spacetelescope/roman_datamodels#550 on all the current regression test files save over them with the result.

roman_datamodels PR spacetelescope/roman_datamodels#555
romancal PR spacetelescope/romancal#1938

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 30, 2025

Codecov Report

❌ Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.48%. Comparing base (db07961) to head (93a8654).
⚠️ Report is 161 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_schemas.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #688      +/-   ##
==========================================
- Coverage   97.70%   97.48%   -0.23%     
==========================================
  Files           8        8              
  Lines         698      716      +18     
==========================================
+ Hits          682      698      +16     
- Misses         16       18       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread latest/meta/product_type.yaml
Copy link
Copy Markdown
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

Should the versions of the tagged scalars themselves be incremented? I know it isn't needed functionally, but it makes sense thematically as this is a meaningful change.

Comment thread latest/reference_files/skycells.yaml Outdated
@PaulHuwe
Copy link
Copy Markdown
Collaborator

PaulHuwe commented Sep 8, 2025

@WilliamJamieson and @braingram - Lets try to ensure that create_from_model gets any adjustments it needs to support converting pre-tag-removal to post.

@WilliamJamieson
Copy link
Copy Markdown
Collaborator Author

Should the versions of the tagged scalars themselves be incremented? I know it isn't needed functionally, but it makes sense thematically as this is a meaningful change.

They effectively have a "new" version because they aren't in tagged_scalars/ they are in meta/. So there is no need for a version change.

Copy link
Copy Markdown
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

LGTM

@WilliamJamieson WilliamJamieson force-pushed the remove_tagged_scalars_v2 branch 2 times, most recently from 0a1cb70 to 4104dc1 Compare September 17, 2025 13:54
@WilliamJamieson WilliamJamieson force-pushed the remove_tagged_scalars_v2 branch 2 times, most recently from 692b562 to 04d0e65 Compare September 18, 2025 17:21
…_type` keyword which was silently getting wrapped by the product_type tagged scalar. For RDM to stop doing that we need to bump its version to signal the change to it.
@WilliamJamieson WilliamJamieson merged commit da422ca into spacetelescope:main Sep 25, 2025
13 of 17 checks passed
@WilliamJamieson WilliamJamieson deleted the remove_tagged_scalars_v2 branch September 25, 2025 17:52
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.

Remove the use of the tagged scalars

2 participants