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

Typo in formulation model constraint #32

Closed
aaraney opened this issue Nov 4, 2022 · 4 comments
Closed

Typo in formulation model constraint #32

aaraney opened this issue Nov 4, 2022 · 4 comments
Assignees
Labels
good first issue Good for newcomers ngen-conf Related to ngen-conf package

Comments

@aaraney
Copy link
Member

aaraney commented Nov 4, 2022

params: "KnownFormulations" = Field(descriminator="model_name")

This should be discriminator. Likewise, the discriminator should be model_type_name not model_name. model_type_name is the alias for model_name, meaning model_type_name is the key in the configuration file.

Edit:

While this was an issue, by correctly spelling discriminator, this forced all model_type_name fields to be type Literal. This is okay in most cases, however it is not desirable for MultiBMI. MultiBMI's model_type_name field should not be constrained like the other BMI model pydantic model types.

However, there still needs to be a way to determine the correct pydantic model variant from the union of formulations, KnownFormulations. The name and model_type_name fields are used for this purpose. name corresponds the bmi adapter name used by that model (e.g. bmi_fortran) and model_type_name roughly corresponds to the model name (e.g. NoahOWP). For non-multi bmi models, the name and model_type_name are constant values (e.g. bmi_fortran and NoahOWP for the NoahOWP pydantic model). In the case of the MultiBMI pydantic model, only name is a constant value, bmi_multi, model_type_name can be any valid string.

However, it feels tedious that it would be required to specify these constants when programmatically creating a for example, a NoahOWP instance. For this reason, these constants are not required when creating a KnownFormulations type. Likewise, it is possible to pass a KnownFormulations instance when creating a Formulation object. However, to deserialized into Formulation object, it is required that name and model_type_name are present for non-multi bmi types and ,potentially only, name it equals bmi_multi`.

@aaraney aaraney added the good first issue Good for newcomers label Nov 4, 2022
@aaraney aaraney added the ngen-conf Related to ngen-conf package label Nov 4, 2022
@aaraney
Copy link
Member Author

aaraney commented Nov 4, 2022

@hellkite500, I can get a PR in for this on Monday. If I forget, just ping me.

@hellkite500
Copy link
Member

@aaraney ping?

@aaraney
Copy link
Member Author

aaraney commented Nov 10, 2022

Thanks for the ping, @hellkite500. Ill get that in now that #30 is merged.

@hellkite500
Copy link
Member

After working through #34, it was decided that model_name as a discriminator isn't required, and has been dropped in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers ngen-conf Related to ngen-conf package
Projects
None yet
Development

No branches or pull requests

2 participants