RCAL-833 Recursively convert all meta attributes during model casting #352
Conversation
The method, as written, was unused throughout the code base. Since the DQInitStep code was in active use, and had just a few modifications, just simply moved that code to the method.
This is needed to ensure tagged scalars get their tags changed to the "more generic" tag.
for more information, see https://pre-commit.ci
|
Attention Reviewers: This is one method; the other is presented in PR #353 This implementation is fixing the previously implemented way of creating the ramp node, update the node with the values from the input model, then create the RampModel from that node. The previous version of this code suffered conversion issues due to the whole of the The implementation in #353 does everything, but directly on the RampModel and not the Ramp stnode. #353 does solve the issue experienced. However, extra keys, such as |
|
@PaulHuwe , can I ask you to take a look at these? Following a discussion with Jonathan, I think either of these are good. I have a vague, poorly defined preference for avoiding any code that knows about nodes, driven mostly by my not really understanding what these are good for, which would argue for #353. On the other hand, I haven't figured out where the existing code avoids updating model_type, so there is some hidden magic via that approach as well---I'm happy either way. Take a look and then we'll let Jonathan decide? Thanks! |
| if isinstance(model, cls): | ||
| return model | ||
| if not isinstance(model, (ScienceRawModel, TvacModel)): |
There was a problem hiding this comment.
Should RampModel be in this list to match the comments?
Also, I think FpsModel should be allowed in this function as well.
There was a problem hiding this comment.
RampModel is implicitly checked for in the preceding statement. However, no issue with adding here also.
tests/test_models.py
Outdated
| def test_rampmodel_from_science_raw(model_class): | ||
| """Test creation of RampModel from raw science/tvac""" | ||
| model = utils.mk_datamodel(model_class) | ||
| ramp = datamodels.RampModel.from_science_raw(model) |
There was a problem hiding this comment.
Maybe add a line where you set a value and confirm the set value is properly propagated?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
- Coverage 97.56% 97.55% -0.01%
==========================================
Files 30 36 +6
Lines 2788 3349 +561
==========================================
+ Hits 2720 3267 +547
- Misses 68 82 +14 ☔ View full report in Codecov by Sentry. |
Resolves RCAL-833
This PR addresses issues found while trying to convert TVAC raw/level 1 data into a RampModel.
The issue is that the meta update was using
dict.update, which doesn't transcribe/copy all the attributes. This left attributes with TVAC tags, producing validation warnings when the RampModel is produced.Solution is to recursively traverse the meta tree, equating/converting each item.
Checklist
CHANGES.rstunder the corresponding subsection