Fix minor bugs with default values and specificity in ngen.cal and ngen.config #34

aaraney · 2022-11-14T16:39:14Z

Fixes minor issues in cal and config's pydantic model fields. This does not introduce any breaking changes.

fixes #31, #32, #33

Ngen Cal [0.2.1] - 2023-06-24

Changes

ngen.cal Objective enum now properly subclasses str. This fixes pydantic models' json schemas that use Objective as a field type. See ngen.cal Objective Enum needs type constraints #31 for impacts.
ngen.cal calibration strategy types: NgenExplicit, NgenIndependent, and NgenUniform, now have default strategy field values.

Ngen Config [0.1.9] - 2023-06-24

Changes

Fixed field discriminator issue in ngen.config's Formulation model params field.
Note, during Formulation object creation if the params field is deserialized (e.g. params's value is a dictionary), the name field is required. If name is not 'bmi_multi', the model_type_name field is also required. Neither are required if a concrete known formulation instance is provided (see Typo in formulation model constraint #32).

Testing

Added tests to verify NgenExplicit, NgenIndependent, and NgenUniform strategy field default value.

Checklist

Testing checklist

Target Environment support

Linux
MacOS

aaraney · 2022-11-14T17:05:58Z

It looks like by fixing the typo in Formulation now the tests are failing. Looking into a resolution to this now.

aaraney · 2022-11-14T17:13:06Z

So, the issue is in the Formulation pydantic model on the params field. Now that the discriminator field is model_name (alias is model_type_name), pydantic wants that field too be a Literal instead of a string. This should be a constant to for Topmod, CFE, NoahOWP, and LSTM but not MultiBMI, correct @hellkite500? Or can we also make it a constant, i.e. a Literal for MultiBMI as well?

hellkite500 · 2022-11-14T17:39:32Z

The multi BMI should be constant at the top level, and the sub modules would be constant in the recursive modules under there

aaraney · 2022-11-14T17:52:44Z

The multi BMI should be constant at the top level, and the sub modules would be constant in the recursive modules under there

So it is safe to tie model_name to a Literal? For concreteness and example, LSTM.model_name should always equal LSTM, @hellkite500?

hellkite500 · 2022-11-14T18:57:16Z

For now I think that will work. That was the intent of the original implementation at least...

aaraney · 2022-11-14T23:21:19Z

For now I think that will work. That was the intent of the original implementation at least...

Just so we are 1000% on the same page, this means that all MultiBMI formulations will have a model_name of BMIMulti.

hellkite500 · 2022-11-18T15:51:51Z

Just so we are 1000% on the same page, this means that all MultiBMI formulations will have a model_name of BMIMulti

As long as that only applies to the container, not the sub formulations, then I don't see a problem with that. (though we can decide if it should be BMIMulti or MultiBMI...haha)

aaraney · 2023-03-30T21:06:52Z

@robertbartel was having some related issues over here which made me revisit this.

The multi BMI should be constant at the top level, and the sub modules would be constant in the recursive modules under there

In re-reading this, @hellkite500, I think I was being a little pedantic in my use of the word 'constant'. To re-phrase, is it okay to enforce that all MultiBMI models have the same model_name field value? Right now, this is not the case model_name is being used to often convey the formulations in the modules field (i.e. "model_type_name": "bmi_multi_noahowp_cfe").

robertbartel · 2023-03-31T13:00:41Z

@aaraney, @hellkite500, I'm only just starting to dig into this, so I may not quite understand everything correctly yet. But ngen-config functionality seems to depend on using specific strings for the model_name of a Formulation object (i.e., the model_type_name element(s) in realization configs).

E.g., a BMI multi-formulation must be configured with something like this in the realization config file:

   "model_type_name": "BMIMulti",

Based on the first few example realization configs I have looked at after beginning to review this (see here), that appears to be a condition that not necessarily followed in practice or in principle, at least in the provided examples. I.e., model_type_name is used to provide a summary description, like bmi_multi_noahowp_cfe.

Additionally, the ngen documentation on BMI formulations in realization configs only indicates that some model_type_name value is required. It doesn't express a need for any specific values. It actually does something closer to the opposite, conveying that the particular value isn't even always important, despite the field itself always being required. I haven't yet checked to see if the ngen code is consistent with the documentation in that manner; perhaps @mattw-nws or @hellkite500 might be able to confirm without having to do a lot of digging.

This probably warrants further design and dependencies discussion, although a verification of my assessments is probably needed first.

aaraney · 2023-03-31T13:42:22Z

@aaraney, @hellkite500, I'm only just starting to dig into this, so I may not quite understand everything correctly yet. But ngen-config functionality seems to depend on using specific strings for the model_name of a Formulation object (i.e., the model_type_name element(s) in realization configs).
...
This probably warrants further design and dependencies discussion, although a verification of my assessments is probably needed first.

@robertbartel, your assessment is correct. ngen.config uses the model_name name field to determine the appropriate model variant for a given formulation (e.g. PET, NoahOWP). From my understanding, this is purely a pragmatic solution as there is not another constant unique field across all models.

robertbartel · 2023-04-04T16:27:04Z

@aaraney, I rebased this branch, then fixed a few other non-Literal model_type_name fields, and I was able to get tests passing locally for the related dmod#316. I have that in my fork, under https://github.com/robertbartel/ngen-cal/tree/pr/34/rebased, if that's of any use to this issue.

aaraney · 2023-04-04T17:35:10Z

@robertbartel, thanks for the breadcrumbs. Ill have a look to see what changes I might need to make in this PR.

aaraney · 2023-04-04T17:45:58Z

@robertbartel, I see. This PR predates the SLOTH module, so it was not added in this PR. That makes sense why it was not working for you.

aaraney · 2023-04-04T18:22:06Z

I added support for SLOTH and PET plus fixed a test that was failing.

aaraney · 2023-07-24T17:45:44Z

The tests should fail because of #62.

aaraney · 2023-07-24T17:50:02Z

The only tests are that failing are because of #62. Ill get in a quick PR to fix that, we can rebase this once that fix is in and re-review.

…-OWP#33

…minator purposes.

…pe_name fields Note, during object creation if the `params` field is deserialized (e.g. `params`'s value is a dictionary), the `name` field is required. If `name` *is not* 'bmi_multi', the `model_type_name` field is also required. Neither are required if a concrete known formulation instance is provided.

hellkite500 · 2023-07-24T20:37:24Z

python/ngen_conf/src/ngen/config/multi.py

@@ -22,7 +23,7 @@ class MultiBMI(BMIParams, smart_union=True):
    # NOTE this is derived from the list of modules
    main_output_variable: Optional[str]
    #NOTE aliases don't propagate to subclasses, so we have to repeat the alias
-    model_name: Optional[str] = Field(alias="model_type_name")
+    model_name: str = Field(alias="model_type_name")


Could this still be optional?

I don't think so. In the super class it is a str. I think the most sensible thing is to either set a default empty string or leave it as is. Thoughts?

well the validator build_model_name was using the optional None in order to build the name from the named nested modules. We can set a default empty string "" but would need to ensure that validator recognizes that and substitutes the built name.

Yeah, this is one of my gripes with pydantic. There is not a semantic way to signify to the caller that something is computed if it is not provided. The way that we have been doing this in DMOD is to set the default of a non-optional field to None. I guess in a perfect world, we would create a constant, say COMPUTED and use that as the default value.

Here, I just went with setting the default value to None and updated the root validator to always set the model name to something if it is not provided.

hellkite500 · 2023-07-24T20:39:20Z

python/ngen_conf/tests/data/test_config.json

@@ -6,7 +6,7 @@
                "name": "bmi_multi", 
                "params": {
                    "name": "bmi_multi", 
-                    "model_type_name": "NoahOWP_CFE", 
+                    "model_type_name": "BMIMulti", 


Should we revert this if "BMIMulti" isn't the required literal now?

That's fine with me. Good catch

aaraney mentioned this pull request Mar 30, 2023

PartialRealizationConfig class and usage in requests and dataservice NOAA-OWP/DMOD#316

Merged

robertbartel mentioned this pull request Apr 4, 2023

Re-activate unit tests for PartialRealizationConfig NOAA-OWP/DMOD#317

Open

aaraney force-pushed the pydantic_fixes branch from b2d77b5 to fa58810 Compare April 4, 2023 17:50

aaraney mentioned this pull request Jun 9, 2023

Test PartialRealizationConfig's is_env_workaround type coercion NOAA-OWP/DMOD#357

Merged

aaraney force-pushed the pydantic_fixes branch from 90e073e to 261dd3c Compare July 24, 2023 14:44

aaraney added 9 commits July 24, 2023 16:35

Fix typo in Formulation, params, field constrains. fixes NOAA-OWP#32.

e3c93ae

add newline to end of file.

9afadf5

Give calibration strategy model's strategy field defaults. fixes NOAA…

b2cc98c

…-OWP#33

test that ngen strategies are constructed with correct default strategy

28f88f5

set default stategy

0aa016f

Formulation model_name's are now Literals. This is required for disri…

ec55543

…minator purposes.

fix merge conflict

1f31884

update tests

4c968d7

add model_name literal consts to SLOTH and PET

4821627

aaraney added 7 commits July 24, 2023 16:35

bmi multi test data needed model_type_name = 'BMIMulti'

e0058a9

add missing Literal import

33c434a

multibmi's model_name field is an unconstrained str

14951bb

add model_type_name and name params to test fixtures

904c2ca

add formulation tests

a7aca5f

re-add model_type_name alias def

98a6372

aaraney force-pushed the pydantic_fixes branch from 1b533a9 to 98a6372 Compare July 24, 2023 20:35

aaraney added 2 commits July 24, 2023 16:38

update changelog

3d6a8da

bump ngen.config version 0.1.8 -> 0.1.9

8befaa0

hellkite500 reviewed Jul 24, 2023

View reviewed changes

update changelog

13d9d57

hellkite500 previously approved these changes Jul 25, 2023

View reviewed changes

aaraney added 3 commits July 25, 2023 10:31

revert model_type_name

9542f32

if no modules or model name are provided, model name is empty string

9847a78

bump ngen.config version 0.2.0 -> 0.2.1

fe0e5a3

aaraney dismissed hellkite500’s stale review via fe0e5a3 July 25, 2023 14:31

aaraney force-pushed the pydantic_fixes branch from 2f81833 to fe0e5a3 Compare July 25, 2023 14:31

hellkite500 approved these changes Jul 25, 2023

View reviewed changes

aaraney merged commit 9d44a00 into NOAA-OWP:master Jul 25, 2023
13 checks passed

aaraney deleted the pydantic_fixes branch July 25, 2023 14:33

hellkite500 mentioned this pull request Jul 25, 2023

Typo in formulation model constraint #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix minor bugs with default values and specificity in ngen.cal and ngen.config #34

Fix minor bugs with default values and specificity in ngen.cal and ngen.config #34

aaraney commented Nov 14, 2022 •

edited

Loading

aaraney commented Nov 14, 2022

aaraney commented Nov 14, 2022 •

edited

Loading

hellkite500 commented Nov 14, 2022

aaraney commented Nov 14, 2022 •

edited

Loading

hellkite500 commented Nov 14, 2022

aaraney commented Nov 14, 2022

hellkite500 commented Nov 18, 2022 •

edited by aaraney

Loading

aaraney commented Mar 30, 2023

robertbartel commented Mar 31, 2023 •

edited

Loading

aaraney commented Mar 31, 2023

robertbartel commented Apr 4, 2023

aaraney commented Apr 4, 2023

aaraney commented Apr 4, 2023

aaraney commented Apr 4, 2023

aaraney commented Jul 24, 2023

aaraney commented Jul 24, 2023

hellkite500 Jul 24, 2023

aaraney Jul 24, 2023

hellkite500 Jul 24, 2023

aaraney Jul 25, 2023

hellkite500 Jul 24, 2023

aaraney Jul 24, 2023

Fix minor bugs with default values and specificity in ngen.cal and ngen.config #34

Fix minor bugs with default values and specificity in ngen.cal and ngen.config #34

Conversation

aaraney commented Nov 14, 2022 • edited Loading

Ngen Cal [0.2.1] - 2023-06-24

Changes

Ngen Config [0.1.9] - 2023-06-24

Changes

Testing

Checklist

Testing checklist

Target Environment support

aaraney commented Nov 14, 2022

aaraney commented Nov 14, 2022 • edited Loading

hellkite500 commented Nov 14, 2022

aaraney commented Nov 14, 2022 • edited Loading

hellkite500 commented Nov 14, 2022

aaraney commented Nov 14, 2022

hellkite500 commented Nov 18, 2022 • edited by aaraney Loading

aaraney commented Mar 30, 2023

robertbartel commented Mar 31, 2023 • edited Loading

aaraney commented Mar 31, 2023

robertbartel commented Apr 4, 2023

aaraney commented Apr 4, 2023

aaraney commented Apr 4, 2023

aaraney commented Apr 4, 2023

aaraney commented Jul 24, 2023

aaraney commented Jul 24, 2023

hellkite500 Jul 24, 2023

Choose a reason for hiding this comment

aaraney Jul 24, 2023

Choose a reason for hiding this comment

hellkite500 Jul 24, 2023

Choose a reason for hiding this comment

aaraney Jul 25, 2023

Choose a reason for hiding this comment

hellkite500 Jul 24, 2023

Choose a reason for hiding this comment

aaraney Jul 24, 2023

Choose a reason for hiding this comment

aaraney commented Nov 14, 2022 •

edited

Loading

aaraney commented Nov 14, 2022 •

edited

Loading

aaraney commented Nov 14, 2022 •

edited

Loading

hellkite500 commented Nov 18, 2022 •

edited by aaraney

Loading

robertbartel commented Mar 31, 2023 •

edited

Loading