From efa8b9ac50839c849fffe825576464d943c0002b Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 09:58:19 -0400 Subject: [PATCH 1/7] refactor: remove redundant logic --- python/ngen_cal/src/ngen/cal/ngen.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/python/ngen_cal/src/ngen/cal/ngen.py b/python/ngen_cal/src/ngen/cal/ngen.py index d4f7a4c4..88a362b6 100644 --- a/python/ngen_cal/src/ngen/cal/ngen.py +++ b/python/ngen_cal/src/ngen/cal/ngen.py @@ -288,7 +288,7 @@ def check_for_partitions(cls, values: dict): raise ValueError("Must provide partitions if using parallel") return values - @root_validator() + @root_validator def validate_hydrofabic(cls, values: dict) -> dict: """ Validates hydrofabric information is provided either as (deprecated) GeoJSON @@ -313,13 +313,6 @@ def validate_hydrofabic(cls, values: dict) -> dict: "or proide catchment, nexus, and crosswalk geojson files." raise ValueError(msg) - if hf is not None: - try: - p = Path(hf) - if not p.exists(): - raise - except: - raise TypeError("hydrofabric must be a valid file path") if cats is not None or nex is not None or x is not None: warnings.warn("GeoJSON support will be deprecated in a future release, use geopackage hydrofabric.", DeprecationWarning) From 446af0abac89b8c2ac65a3f79e57e656419f33c7 Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 10:03:15 -0400 Subject: [PATCH 2/7] fix: routing_output cannot be None --- python/ngen_cal/src/ngen/cal/ngen.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ngen_cal/src/ngen/cal/ngen.py b/python/ngen_cal/src/ngen/cal/ngen.py index 88a362b6..3847740f 100644 --- a/python/ngen_cal/src/ngen/cal/ngen.py +++ b/python/ngen_cal/src/ngen/cal/ngen.py @@ -83,7 +83,7 @@ class NgenBase(ModelExec): nexus: Optional[FilePath] crosswalk: Optional[FilePath] ngen_realization: Optional[NgenRealization] - routing_output: Optional[Path] = Field(default=Path("flowveldepth_Ngen.csv")) + routing_output: Path = Path("flowveldepth_Ngen.csv") #optional fields partitions: Optional[FilePath] parallel: Optional[PosInt] From 2034f83f500b8b80e046d38b4b4037684e37377d Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 11:18:05 -0400 Subject: [PATCH 3/7] feat: add errors module --- python/ngen_cal/src/ngen/cal/errors.py | 1 + 1 file changed, 1 insertion(+) create mode 100644 python/ngen_cal/src/ngen/cal/errors.py diff --git a/python/ngen_cal/src/ngen/cal/errors.py b/python/ngen_cal/src/ngen/cal/errors.py new file mode 100644 index 00000000..c3be7f0b --- /dev/null +++ b/python/ngen_cal/src/ngen/cal/errors.py @@ -0,0 +1 @@ +class UnsupportedFeatureError(ValueError): ... From 1c9970c030b7de66d50bde30bcb33f03e32805fa Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 11:21:12 -0400 Subject: [PATCH 4/7] refactor: split up root validator logic --- python/ngen_cal/src/ngen/cal/ngen.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/python/ngen_cal/src/ngen/cal/ngen.py b/python/ngen_cal/src/ngen/cal/ngen.py index 3847740f..bb6edbdb 100644 --- a/python/ngen_cal/src/ngen/cal/ngen.py +++ b/python/ngen_cal/src/ngen/cal/ngen.py @@ -28,6 +28,7 @@ from hypy.nexus import Nexus from hypy.catchment import Catchment + class NgenStrategy(str, Enum): """ """ @@ -289,19 +290,24 @@ def check_for_partitions(cls, values: dict): return values @root_validator - def validate_hydrofabic(cls, values: dict) -> dict: + def _validate_model(cls, values: dict) -> dict: + NgenBase._verify_hydrofabric(values) + return values + + @staticmethod + def _verify_hydrofabric(values: dict) -> None: """ - Validates hydrofabric information is provided either as (deprecated) GeoJSON + Verify hydrofabric information is provided either as (deprecated) GeoJSON or (preferred) GeoPackage files. Args: - values (dict): configuation values to check + values: root validator dictionary Raises: ValueError: If a geopackage hydrofabric or set of geojsons are not found Returns: - dict: Valid configuration elements for hydrofabric requirements + None """ hf: FilePath = values.get('hydrofabric') cats: FilePath = values.get("catchments") From a6307f9d9f560378f8fc0622fdacbb6b3c0a9a0d Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 11:22:44 -0400 Subject: [PATCH 5/7] feat: raise if realization 'output_root' is set. close #151 --- python/ngen_cal/src/ngen/cal/ngen.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/python/ngen_cal/src/ngen/cal/ngen.py b/python/ngen_cal/src/ngen/cal/ngen.py index bb6edbdb..90fdabdb 100644 --- a/python/ngen_cal/src/ngen/cal/ngen.py +++ b/python/ngen_cal/src/ngen/cal/ngen.py @@ -292,6 +292,8 @@ def check_for_partitions(cls, values: dict): @root_validator def _validate_model(cls, values: dict) -> dict: NgenBase._verify_hydrofabric(values) + realization: Optional[NgenRealization] = values.get("ngen_realization") + NgenBase._verify_ngen_realization(realization) return values @staticmethod @@ -322,7 +324,29 @@ def _verify_hydrofabric(values: dict) -> None: if cats is not None or nex is not None or x is not None: warnings.warn("GeoJSON support will be deprecated in a future release, use geopackage hydrofabric.", DeprecationWarning) - return values + @staticmethod + def _verify_ngen_realization(realization: Optional[NgenRealization]) -> None: + """ + Verify `ngen_realization` uses supported features. + + Args: + realization: maybe an `NgenRealization` instance + + Raises: + UnsupportedFeatureError: If `realization.output_root` is not None. + Feature not supported. + + Returns: + None + """ + if realization is None: + return None + + if realization.output_root is not None: + from .errors import UnsupportedFeatureError + raise UnsupportedFeatureError( + "ngen realization `output_root` field is not supported by ngen.cal. will be removed in future; see https://github.com/NOAA-OWP/ngen-cal/issues/150" + ) def update_config(self, i: int, params: 'pd.DataFrame', id: str = None, path=Path("./")): """_summary_ From 8c308b219670c871a44246d94f90dbd9d9643ed3 Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 11:23:12 -0400 Subject: [PATCH 6/7] style: formatting --- python/ngen_cal/tests/test_ngen.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/python/ngen_cal/tests/test_ngen.py b/python/ngen_cal/tests/test_ngen.py index ad7f826d..e83c68ff 100644 --- a/python/ngen_cal/tests/test_ngen.py +++ b/python/ngen_cal/tests/test_ngen.py @@ -1,5 +1,15 @@ +import pathlib + import pytest -from ngen.cal.ngen import NgenExplicit, NgenIndependent, NgenUniform, NgenStrategy +import pydantic +from ngen.cal.ngen import ( + Ngen, + NgenBase, + NgenExplicit, + NgenIndependent, + NgenStrategy, + NgenUniform, +) def test_NgenExplicit_strategy_default_value(): From ed25351703854f311d089307829e28d0886cea46 Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Tue, 13 Aug 2024 11:23:36 -0400 Subject: [PATCH 7/7] test: validation error raised if 'output_root' provided --- python/ngen_cal/tests/test_ngen.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/python/ngen_cal/tests/test_ngen.py b/python/ngen_cal/tests/test_ngen.py index e83c68ff..cab77249 100644 --- a/python/ngen_cal/tests/test_ngen.py +++ b/python/ngen_cal/tests/test_ngen.py @@ -23,7 +23,20 @@ def test_NgenIndependent_strategy_default_value(): 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 + + +def test_NgenBase_verify_realization(ngen_config: Ngen): + # session level pytest fixture. take deep copy to avoid pollution + config = ngen_config.__root__.copy(deep=True) + assert isinstance(config, NgenBase) + + assert config.ngen_realization is not None, "should have already raised if not None" + config.ngen_realization.output_root = pathlib.Path("./output_root") + + with pytest.raises(pydantic.ValidationError): + Ngen.parse_obj(dict(config))