Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCAL-728: Fix meta.model_type to reflect the data model's type #296

Merged

Conversation

WilliamJamieson
Copy link
Collaborator

@WilliamJamieson WilliamJamieson commented Dec 8, 2023

Resolves RCAL-728

Closes spacetelescope/romancal#1036

This PR ensures that during model initialization, the meta.model_type is set to reflect the "model_type" of the datamodel. This does not enforce this past initialization.

Note, I chose "model_type" to mean the "name" (__class__.__name__) of the RDM datamodel. This is an arbitrary minimal choice. A reasonable alternative is the CamelCase-ification of the root name of the top level schema, eg .../wfi_science_raw -> WfiScienceRawModel or WfiScienceRaw, but the __class__.__name__ corresponding to this is acutally ScienceRawModel. In any case, we need to clarify our model-naming conventions and decide how they map to the meta.model_type.

Checklist

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (da87fdc) 97.17% compared to head (8724eee) 96.98%.

❗ Current head 8724eee differs from pull request most recent head 5e8515f. Consider uploading reports for the commit 5e8515f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
- Coverage   97.17%   96.98%   -0.20%     
==========================================
  Files          28       28              
  Lines        2516     2521       +5     
==========================================
  Hits         2445     2445              
- Misses         71       76       +5     

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

super().__init__(init, **kwargs)

if init is not None:
self.meta.model_type = self.__class__.__name__
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This decides that the meta.model_type string should be the name of the model

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is correct. model_type should be the name of the datamodel to instantiate.

@nden
Copy link
Collaborator

nden commented Dec 14, 2023

While I think this PR addresses the issue, looking at the current state of the schemas I am a bit confused. I see spacetelescope/rad#278 added a datamodel_object field to the schemas which, I think, duplicates what model_type was supposed to be. OTH model_type is added to science files but not to reference files.

@WilliamJamieson
Copy link
Collaborator Author

WilliamJamieson commented Dec 14, 2023

While I think this PR addresses the issue, looking at the current state of the schemas I am a bit confused. I see spacetelescope/rad#278 added a datamodel_object field to the schemas which, I think, duplicates what model_type was supposed to be. OTH model_type is added to science files but not to reference files.

I think you mean datamodel_name. This has nothing to do with model_type directly. datamodel_name was added to RAD in order to do two things which have nothing to do with the contents of the models:

  1. Indicate by its existence if a schema describes a "top-level" schema that describes a node which will be directly wrapped by a data model.
  2. For purposes of cross checking between rad and roman_datamodels in addition to its existence indicating a data model should exist corresponding to the schema, it acts as an indication of which data model is doing so.

For both purposes, these exist only to enable testing in roman_datamodels to ensure that it is consistent with the expectation of rad because we do not have any organizational marking of which schemas are expected to be wrapped by a data model, nor do we currently have any consistency between data model wrappers and the model names.

Note that in addition to the above datamodel_name exists above the object properties definition, meaning that the data models cannot directly access this field in the schema.

Note that as the schemas are currently structured, model_type cannot be used for the same purpose as datamodel_name. This is because it is buried deep within the meta field schema logic itself. Even if it marked the model somewhat like what is done with reftype for the reference files, it would suffer from the same issue as reftype; namely, it is not easily accessible from direct schema parsing. That is it requires a look down both through the meta field AND an allOf meaning its an extremely brittle indication. datamodel_name being in the very top level means it acts like id for example, meaning it will always be accessible without any extra parsing logic, only needing to read the file.

@WilliamJamieson
Copy link
Collaborator Author

It just occurred to me that an alternate/additional solution to the problem this PR is attempting to solve is to do something similar to #295. Namely, set meta.model_type correctly as part of writing the model to a file. This ensures that written files will always have meta.model_type correctly set in a written file.

Doing this alone has the drawback that meta.model_type will not necessarily be a reliable indication of the model's type when running the code. However, realistically type(model) should be preferred when actually running the code. Moreover, note that meta.model_type is redundant information as the model's type is already directly encoded into the ASDF file via the ASDF tag placed on the tree representing the model in the file. The only benefit it actually has is that it is decorated in the ASDF schema with archive related metadata.

@WilliamJamieson WilliamJamieson merged commit 29f0cf9 into spacetelescope:main Dec 15, 2023
12 checks passed
@WilliamJamieson WilliamJamieson deleted the bugfix/model_type branch December 15, 2023 17:32
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.

model_type needs to be updated
3 participants