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

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

Merged
merged 22 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e3c93ae
Fix typo in Formulation, params, field constrains. fixes #32.
aaraney Nov 14, 2022
9afadf5
add newline to end of file.
aaraney Nov 14, 2022
b2cc98c
Give calibration strategy model's strategy field defaults. fixes #33
aaraney Nov 14, 2022
28f88f5
test that ngen strategies are constructed with correct default strategy
aaraney Nov 14, 2022
0aa016f
set default stategy
aaraney Jul 24, 2023
ec55543
Formulation model_name's are now Literals. This is required for disri…
aaraney Nov 14, 2022
1f31884
fix merge conflict
aaraney Jul 24, 2023
4c968d7
update tests
aaraney Nov 14, 2022
4821627
add model_name literal consts to SLOTH and PET
aaraney Apr 4, 2023
e0058a9
bmi multi test data needed model_type_name = 'BMIMulti'
aaraney Apr 4, 2023
33c434a
add missing Literal import
aaraney Jul 24, 2023
7582871
formulation types are now discriminated using their name and model_ty…
aaraney Jul 24, 2023
14951bb
multibmi's model_name field is an unconstrained str
aaraney Jul 24, 2023
904c2ca
add model_type_name and name params to test fixtures
aaraney Jul 24, 2023
a7aca5f
add formulation tests
aaraney Jul 24, 2023
98a6372
re-add model_type_name alias def
aaraney Jul 24, 2023
3d6a8da
update changelog
aaraney Jul 24, 2023
8befaa0
bump ngen.config version 0.1.8 -> 0.1.9
aaraney Jul 24, 2023
13d9d57
update changelog
aaraney Jul 24, 2023
9542f32
revert model_type_name
aaraney Jul 24, 2023
9847a78
if no modules or model name are provided, model name is empty string
aaraney Jul 25, 2023
fe0e5a3
bump ngen.config version 0.2.0 -> 0.2.1
aaraney Jul 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions python/ngen_cal/src/ngen/cal/ngen.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def update_config(self, i: int, params: 'pd.DataFrame', id: str = None, path=Pat

class NgenExplicit(NgenBase):

strategy: Literal[NgenStrategy.explicit]
strategy: Literal[NgenStrategy.explicit] = NgenStrategy.explicit

def __init__(self, **kwargs):
#Let pydantic work its magic
Expand Down Expand Up @@ -294,7 +294,7 @@ def update_config(self, i: int, params: 'pd.DataFrame', id: str, **kwargs):

class NgenIndependent(NgenBase):
# TODO Error if not routing block in ngen_realization
strategy: Literal[NgenStrategy.independent]
strategy: Literal[NgenStrategy.independent] = NgenStrategy.independent
params: Mapping[str, Parameters] #required in this case...

def __init__(self, **kwargs):
Expand Down Expand Up @@ -381,7 +381,7 @@ class NgenUniform(NgenBase):
which is applied to each catchment in the hydrofabric being simulated.
"""
# TODO Error if not routing block in ngen_realization
strategy: Literal[NgenStrategy.uniform]
strategy: Literal[NgenStrategy.uniform] = NgenStrategy.uniform
params: Mapping[str, Parameters] #required in this case...

def __init__(self, **kwargs):
Expand Down Expand Up @@ -455,4 +455,4 @@ def resolve_paths(self, relative_to: Optional[Path]=None):

@property
def best_params(self):
return self.__root__.eval_params.best_params
return self.__root__.eval_params.best_params
2 changes: 1 addition & 1 deletion python/ngen_cal/src/ngen/cal/strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,4 @@ class Sensitivity(BaseModel):
NOT IMPLEMENTED
"""
type: Literal['sensitivity']
pass #Not Implemented
pass #Not Implemented
19 changes: 19 additions & 0 deletions python/ngen_cal/tests/test_ngen.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import pytest
from ngen.cal.ngen import NgenExplicit, NgenIndependent, NgenUniform, NgenStrategy


def test_NgenExplicit_strategy_default_value():
# construct object without validation.
o = NgenExplicit.construct()
assert o.strategy == NgenStrategy.explicit


def test_NgenIndependent_strategy_default_value():
# construct object without validation.
o = NgenIndependent.construct()
assert o.strategy == NgenStrategy.independent

def test_NgenUniform_strategy_default_value():
# construct object without validation.
o = NgenUniform.construct()
assert o.strategy == NgenStrategy.uniform
4 changes: 2 additions & 2 deletions python/ngen_conf/src/ngen/config/cfe.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Optional
from typing import Optional, Literal
from pydantic import BaseModel, Field

from .bmi_formulation import BMIC
Expand All @@ -20,7 +20,7 @@ class CFE(BMIC):
main_output_variable: str = 'Q_OUT'
registration_function: str = "register_bmi_cfe"
#NOTE aliases don't propagate to subclasses, so we have to repeat the alias
model_name: str = Field("CFE", const=True, alias="model_type_name")
model_name: Literal["CFE"] = Field("CFE", const=True, alias="model_type_name")

#can set some default name map entries...will be overridden at construction
#if a name_map with the same key is passed in, otherwise the name_map
Expand Down
34 changes: 29 additions & 5 deletions python/ngen_conf/src/ngen/config/formulation.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,39 @@
from pydantic import BaseModel, Field
from typing import TYPE_CHECKING
from pydantic import BaseModel, validator
from typing import Any, Dict, TYPE_CHECKING, Union
if TYPE_CHECKING:
from typing import Optional
from pathlib import Path

class Formulation(BaseModel, smart_union=True):
"""Model of an ngen formulation
"""
Model of an ngen formulation.

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.
"""
#TODO make this an enum?
name: str
params: "KnownFormulations" = Field(descriminator="model_name")
params: "KnownFormulations"

@validator("params", pre=True)
def _validate_params(
cls, value: Union[Dict[str, Any], "KnownFormulations"]
) -> Union[Dict[str, Any], "KnownFormulations"]:
if isinstance(value, BaseModel):
return value

if isinstance(value, dict):
name = value.get("name")
if name == "bmi_multi":
return value

# non-bmi multi subtypes must specify their `name` and `model_type_name` fields when _deserializing_.
# note: this does not apply if a subtype instance is provided
if name is None and ("model_type_name" not in value or "model_name" not in value):
raise ValueError("'name' and 'model_type_name' fields are required when deserializing _into_ a 'Formulation'.")
return value

def resolve_paths(self, relative_to: 'Optional[Path]'=None):
self.params.resolve_paths(relative_to)
Expand All @@ -21,4 +45,4 @@ def resolve_paths(self, relative_to: 'Optional[Path]'=None):
# So we defer type cheking and importing the KnownFormulations until after
#MultiBMI is defined, then update_forward_refs()
from .all_formulations import KnownFormulations
Formulation.update_forward_refs()
Formulation.update_forward_refs()
2 changes: 1 addition & 1 deletion python/ngen_conf/src/ngen/config/lstm.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class LSTM(BMIPython):
python_type: Union[PyObject, str] = "bmi_lstm.bmi_LSTM"
main_output_variable: Literal["land_surface_water__runoff_depth"] = "land_surface_water__runoff_depth"
#NOTE aliases don't propagate to subclasses, so we have to repeat the alias
model_name: str = Field("LSTM", alias="model_type_name")
model_name: Literal["LSTM"] = Field("LSTM", alias="model_type_name")

_variable_names_map = {
"atmosphere_water__time_integral_of_precipitation_mass_flux":"RAINRATE"
Expand Down
12 changes: 10 additions & 2 deletions python/ngen_conf/src/ngen/config/multi.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from typing import Sequence, Mapping, Any, Optional, TYPE_CHECKING
from typing_extensions import Literal
if TYPE_CHECKING:
from pathlib import Path

Expand All @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

Could this still be optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


#override const since these shouldn't be used for multi bmi, but are currently
#required to exist as keys for ngen
Expand Down Expand Up @@ -79,7 +80,14 @@ def pick_main_output(cls, values: Mapping[str, Any]) -> Mapping[str, Any]:
var = values.get('main_output_variable')
modules = values.get('modules')
if not var and modules:
values['main_output_variable'] = modules[-1]['params']['main_output_variable']
last = modules[-1]

# cannot treat Formulation type like dictionary
from ngen.config.formulation import Formulation
if isinstance(last, Formulation):
values['main_output_variable'] = last.params.main_output_variable
else:
values['main_output_variable'] = last['params']['main_output_variable']
return values

#NOTE To avoid circular import and support recrusive modules
Expand Down
3 changes: 2 additions & 1 deletion python/ngen_conf/src/ngen/config/noahowp.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from typing import Literal
from pydantic import BaseModel, Field

from .bmi_formulation import BMIFortran
Expand All @@ -17,7 +18,7 @@ class NoahOWP(BMIFortran):
model_params: NoahOWPParams = None
main_output_variable: str = 'QINSUR'
#NOTE aliases don't propagate to subclasses, so we have to repeat the alias
model_name: str = Field("NoahOWP", const=True, alias="model_type_name")
model_name: Literal["NoahOWP"] = Field("NoahOWP", const=True, alias="model_type_name")

_variable_names_map = {
"PRCPNONC": "atmosphere_water__liquid_equivalent_precipitation_rate",
Expand Down
2 changes: 1 addition & 1 deletion python/ngen_conf/src/ngen/config/pet.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class PET(BMIC):
#should all be reasonable defaults for pET
main_output_variable: Literal["water_potential_evaporation_flux"] = "water_potential_evaporation_flux"
#NOTE aliases don't propagate to subclasses, so we have to repeat the alias
model_name: str = Field("PET", alias="model_type_name")
model_name: Literal["PET"] = Field("PET", alias="model_type_name")
# source: https://github.com/NOAA-OWP/evapotranspiration/blob/0a66999db9695bccf4c1e35d904aa86f04e6cacf/src/bmi_pet.c#L1215
registration_function: str = "register_bmi_pet"

Expand Down
4 changes: 2 additions & 2 deletions python/ngen_conf/src/ngen/config/sloth.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Optional, Mapping
from typing import Literal, Optional, Mapping
from pydantic import BaseModel, Field

from .bmi_formulation import BMICxx
Expand All @@ -9,4 +9,4 @@ class SLOTH(BMICxx):
model_params: Optional[Mapping[str, str]] #Is this a better represntation of SLOTH params??? just generic mappings?
registration_function: str = "none" #FIXME this isn't required for CXX bmi in ngen?
#NOTE aliases don't propagate to subclasses, so we have to repeat the alias
model_name: str = Field("SLOTH", const=True, alias="model_type_name")
model_name: Literal["SLOTH"] = Field("SLOTH", const=True, alias="model_type_name")
4 changes: 2 additions & 2 deletions python/ngen_conf/src/ngen/config/topmod.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Optional
from typing import Optional, Literal
from pydantic import BaseModel, Field

from .bmi_formulation import BMIC
Expand All @@ -19,7 +19,7 @@ class Topmod(BMIC):
main_output_variable: str = 'Qout'
registration_function: str = "register_bmi_topmodel"
#NOTE aliases don't propagate to subclasses, so we have to repeat the alias
model_name: str = Field("TOPMODEL", const=True, alias="model_type_name")
model_name: Literal["TOPMODEL"] = Field("TOPMODEL", const=True, alias="model_type_name")

#can set some default name map entries...will be overridden at construction
#if a name_map with the same key is passed in, otherwise the name_map
Expand Down
56 changes: 42 additions & 14 deletions python/ngen_conf/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
from ngen.config.cfe import CFE
from ngen.config.sloth import SLOTH
from ngen.config.noahowp import NoahOWP
from ngen.config.topmod import Topmod
from ngen.config.lstm import LSTM
from ngen.config.multi import MultiBMI

#set the workdir relative to this test config
Expand Down Expand Up @@ -42,7 +44,10 @@ def routing():
@pytest.fixture
def cfe_params():
path = _workdir.joinpath("data/CFE/")
data = {'config_prefix':path,
data = {
'model_type_name': 'CFE',
'name': 'bmi_c',
'config_prefix':path,
# 'config': "{{id}}_config.txt",
'config': "config.txt",
'library_prefix':path,
Expand All @@ -53,7 +58,10 @@ def cfe_params():
@pytest.fixture
def sloth_params():
path = _workdir.joinpath("data/sloth/")
data = {'config_prefix':path,
data = {
'model_type_name': 'SLOTH',
'name': 'bmi_c++',
'config_prefix':path,
# 'config': "{{id}}_config.txt",
'config': "config.txt",
'library_prefix':path,
Expand All @@ -64,7 +72,10 @@ def sloth_params():
@pytest.fixture
def topmod_params():
path = _workdir.joinpath("data/CFE/")
data = {'config_prefix':path,
data = {
'model_type_name': 'TOPMODEL',
'name': 'bmi_c',
'config_prefix':path,
# 'config': "{{id}}_config.txt",
'config': "config.txt",
'library_prefix':path,
Expand All @@ -76,13 +87,26 @@ def topmod_params():
def noahowp_params():
path = _workdir.joinpath("data/NOAH/")
libpath = _workdir.joinpath("data/CFE")
data = {'config_prefix':path,
data = {
'model_type_name': 'NoahOWP',
'name': 'bmi_fortran',
'config_prefix':path,
'config': "{{id}}.input",
'library_prefix': libpath,
'library': 'libfakecfe.so'
}
return data

@pytest.fixture
def lstm_params():
path = _workdir.joinpath("data/CFE/")
data = {
'model_type_name': 'LSTM',
'name': 'bmi_python',
'config_prefix':path,
'config': "{{id}}_config.txt"}
return data

@pytest.fixture
def cfe(cfe_params):
return CFE(**cfe_params)
Expand All @@ -91,29 +115,33 @@ def cfe(cfe_params):
def sloth(sloth_params):
return SLOTH(**sloth_params)

@pytest.fixture
def topmod(topmod_params):
return Topmod(**topmod_params)

@pytest.fixture
def noahowp(noahowp_params):
return NoahOWP(**noahowp_params)

@pytest.fixture
def multi(cfe, noahowp, forcing):
def lstm(lstm_params):
return LSTM(**lstm_params)

@pytest.fixture
def multi(cfe, noahowp):
cfe.allow_exceed_end_time=True
noahowp.allow_exceed_end_time=True
f1 = Formulation(name=noahowp.name, params=noahowp)
f2 = Formulation(name=cfe.name, params=cfe)
return MultiBMI(modules=[f1.dict(), f2.dict()], allow_exceed_end_time=True)
return MultiBMI(modules=[f1, f2], allow_exceed_end_time=True)

@pytest.fixture
def lstm_params():
path = _workdir.joinpath("data/CFE/")
data = {'config_prefix':path,
'config': "{{id}}_config.txt"}
return data

@pytest.fixture
def multi_params(cfe, noahowp):
data = {"modules":[Formulation(name=noahowp.name, params=noahowp).dict(),
Formulation(name=cfe.name, params=cfe).dict()]
data = {
"name": "bmi_multi",
"modules":[Formulation(name=noahowp.name, params=noahowp).dict(by_alias=True),
Formulation(name=cfe.name, params=cfe).dict(by_alias=True)]
}
return data

Expand Down
2 changes: 1 addition & 1 deletion python/ngen_conf/tests/data/test_config.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"name": "bmi_multi",
"params": {
"name": "bmi_multi",
"model_type_name": "NoahOWP_CFE",
"model_type_name": "BMIMulti",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine with me. Good catch

"main_output_variable": "Q_OUT",
"init_config": "",
"allow_exceed_end_time": false,
Expand Down
Loading
Loading