From 7548f17dbe0ffbb6e1bd0c5c7957bbebfaa5ae32 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 06:58:07 -0600 Subject: [PATCH 01/12] Add .kiro and .vscode to .gitignore --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 16b3098..3e9541f 100644 --- a/.gitignore +++ b/.gitignore @@ -70,3 +70,6 @@ Icon *.swp .idea + +.kiro +.vscode From 0e77f8799b58bf6ab24837f952c525bc8acc6b24 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 06:58:19 -0600 Subject: [PATCH 02/12] Fix handling of bounded parameters - Add 'is_bounded' flag in metadata when parameter min/max is 'bounded' - Resolve bounded parameter maximum to the actual dimension size in Parameter - Add bounded dimension to global dimensions in Parameters.add() - Remove post-load adjust_bounded_parameters() calls from ParameterFile, ParameterNetCDF, and ParamDb (now handled at parameter creation time) - Fix outlier detection to skip string-type parameters instead of checking for string min/max values - Use deepcopy for metadata and dimensions to prevent cross-contamination - Fix whitespace alignment in ParamDb --- pyPRMS/metadata/metadata.py | 1 + pyPRMS/parameters/ParamDb.py | 6 ++-- pyPRMS/parameters/Parameter.py | 36 ++++++++++++++++------- pyPRMS/parameters/ParameterFile.py | 2 -- pyPRMS/parameters/ParameterNetCDF.py | 4 +-- pyPRMS/parameters/Parameters.py | 44 ++++++++++++++++++++-------- 6 files changed, 61 insertions(+), 32 deletions(-) diff --git a/pyPRMS/metadata/metadata.py b/pyPRMS/metadata/metadata.py index e095115..ccca4cb 100644 --- a/pyPRMS/metadata/metadata.py +++ b/pyPRMS/metadata/metadata.py @@ -297,6 +297,7 @@ def _parameters_to_dict(self, xml_root: xmlET.Element, except ValueError: if text == 'bounded': meta_dict[name][ek] = meta_dict[name]['default'] + meta_dict[name]['is_bounded'] = True else: meta_dict[name][ek] = text else: diff --git a/pyPRMS/parameters/ParamDb.py b/pyPRMS/parameters/ParamDb.py index 51d2775..ede4372 100644 --- a/pyPRMS/parameters/ParamDb.py +++ b/pyPRMS/parameters/ParamDb.py @@ -62,8 +62,8 @@ def _read(self): # Create a MetaData object to use its parameter parsing function mobj = MetaData() pvt_meta = mobj._parameters_to_dict(xml_root=params_root, - meta_type='parameters', - req_version=PRMS_VERSION) + meta_type='parameters', + req_version=PRMS_VERSION) # Populate parameterSet with all available parameter names for param in params_root.findall('parameter'): @@ -92,5 +92,3 @@ def _read(self): self.get(xml_param_name).data = tmp_data else: con.print(f'[orange3]WARNING[/]: {xml_param_name}, ParamDb file does not exist; skipping') - - self.adjust_bounded_parameters() diff --git a/pyPRMS/parameters/Parameter.py b/pyPRMS/parameters/Parameter.py index 7c96bb0..ec80adb 100644 --- a/pyPRMS/parameters/Parameter.py +++ b/pyPRMS/parameters/Parameter.py @@ -11,7 +11,7 @@ from ..base.console import get_console_instance from ..constants import NEW_PTYPE_TO_DTYPE from ..dimensions.Dimensions import ParamDimensions -from ..Exceptions_custom import FixedDimensionError +from ..Exceptions_custom import FixedDimensionError, ParameterNotValidError con = None @@ -75,13 +75,27 @@ def __init__(self, name: str, for cname in self.meta['dimensions']: self.__dimensions.add(cname) + # TODO: 2026-08-12 PAN - if this is a bounded parameter and global_dims + # was not passed then what should happen? an error? a warning? nothing? + # if self.meta.get('maximum') in list(self.dimensions.keys()): + if self.meta.get('is_bounded', False): + if global_dims is None: + raise ParameterNotValidError(f'Parameter, {self.name}, is bounded but no global dimensions were supplied') + + # Save the name of the bounded-dimension + self.meta['bounded_dimension_name'] = self.meta.get('maximum') + self.meta['maximum'] = global_dims.get(self.meta.get('bounded_dimension_name')).size + + if self.__verbose: # pragma: no cover + con.print(f'[bold]{self.name}[/]: valid upper bound adjusted to {self.meta["maximum"]}') + if global_dims is not None: self.__dimensions[cname].size = global_dims.get(cname).size self.__dimensions[cname].meta = global_dims[cname].meta else: raise ValueError(f'`{self.name}` does not exist in metadata') else: - # The meta must be supplied as an adhoc dictionary + # The metadata must be supplied as an adhoc dictionary self.meta = meta self.__data: ParamDataRawType | None = None @@ -353,12 +367,11 @@ def check_values(self) -> bool: minval = self.meta.get('minimum', None) maxval = self.meta.get('maximum', None) - if minval is not None and maxval is not None: - # Check both ends of the range - if not (isinstance(minval, str) or isinstance(maxval, str)): + if self.meta.get('datatype') != 'string': + if minval is not None and maxval is not None: + # Check both ends of the range + # if not (isinstance(minval, str) or isinstance(maxval, str)): return (self.data_raw >= minval).all() and (self.data_raw <= maxval).all().item() - elif minval == 'bounded': - return (self.data_raw >= self.meta.get('default')).all().item() # type: ignore return True @@ -407,11 +420,12 @@ def outliers(self) -> Outliers: values_under = 0 values_over = 0 - if self.meta.get('minimum', None) is not None: - values_under = np.count_nonzero(self.data_raw < self.meta.get('minimum')) # type: ignore + if self.meta.get('datatype') != 'string': + if self.meta.get('minimum', None) is not None: + values_under = np.count_nonzero(self.data_raw < self.meta.get('minimum')) # type: ignore - if self.meta.get('maximum', None) is not None: - values_over = np.count_nonzero(self.data_raw > self.meta.get('maximum')) # type: ignore + if self.meta.get('maximum', None) is not None: + values_over = np.count_nonzero(self.data_raw > self.meta.get('maximum')) # type: ignore return Outliers(self.__name, values_under, values_over) diff --git a/pyPRMS/parameters/ParameterFile.py b/pyPRMS/parameters/ParameterFile.py index b90f341..9306555 100644 --- a/pyPRMS/parameters/ParameterFile.py +++ b/pyPRMS/parameters/ParameterFile.py @@ -179,6 +179,4 @@ def _read(self): pass self.get(varname).data = vals # type: ignore - - self.adjust_bounded_parameters() self.__isloaded = True diff --git a/pyPRMS/parameters/ParameterNetCDF.py b/pyPRMS/parameters/ParameterNetCDF.py index 42df42c..265a0be 100644 --- a/pyPRMS/parameters/ParameterNetCDF.py +++ b/pyPRMS/parameters/ParameterNetCDF.py @@ -76,6 +76,4 @@ def _read(self): self.add(name=str(var)) # Add the data - self.get(str(var)).data = cparam.values - - self.adjust_bounded_parameters() \ No newline at end of file + self.get(str(var)).data = cparam.values \ No newline at end of file diff --git a/pyPRMS/parameters/Parameters.py b/pyPRMS/parameters/Parameters.py index be3ffc4..42e628a 100644 --- a/pyPRMS/parameters/Parameters.py +++ b/pyPRMS/parameters/Parameters.py @@ -13,6 +13,7 @@ import xml.dom.minidom as minidom import xml.etree.ElementTree as xmlET +from copy import deepcopy from collections import defaultdict from collections.abc import KeysView, Sequence from functools import cached_property @@ -66,7 +67,10 @@ def __init__(self, metadata: MetaDataType, con = get_console_instance() # con.print('Parameters: Console info: {}'.format(con)) - self.__dimensions = Dimensions(metadata=metadata, verbose=verbose) + # A full, separate copy of the original metadata dictionary + self.__full_metadata = deepcopy(metadata) + + self.__dimensions = deepcopy(Dimensions(metadata=metadata, verbose=verbose)) self.__parameters: dict[str, Parameter] = dict() self.verbose = verbose @@ -77,7 +81,6 @@ def __init__(self, metadata: MetaDataType, self.__seg_shape_key: str | None = None self.__seg_to_hru: dict[int, list[int]] = dict() self.__hru_to_seg: dict[int, int] = dict() - self.__full_metadata = metadata self.metadata = metadata['parameters'] self.prms_version = Version(metadata['info']['version']) @@ -331,7 +334,7 @@ def add(self, name: str): :param name: A valid PRMS parameter name - :raises ParameterError: if parameter already exists or name is None + :raises ParameterError: If the parameter already exists or name is None """ # Add a new parameter @@ -345,6 +348,23 @@ def add(self, name: str): if not self.__dimensions.exists(cdim): raise KeyError(f'Global dimension, {cdim}, does not exist') + if self.metadata[name].get('is_bounded', False): + # Add the upper-bound dimension to the global dimensions + bounded_dim_name = self.metadata[name]['maximum'] + if not self.__dimensions.exists(bounded_dim_name): + if bounded_dim_name == 'ndepl': + # This is the one dimension where the size depends on another dimension + self.dimensions.add(name=bounded_dim_name, size=int(self.dimensions.get('ndeplval').size / 11)) + elif bounded_dim_name == 'nobs': + # If this is missing it should be added with the same value as npoigages. + self.dimensions.add(name=bounded_dim_name, size=self.dimensions.get('npoigages').size) + else: + self.__dimensions.add(bounded_dim_name) + + con.print(f'[orange3]WARNING[/]: Bounded parameter, {name}, requires dimension, {bounded_dim_name}, ' + f'which is missing from global dimensions; ' + f'added with size = {self.__dimensions.get(bounded_dim_name).size}') + self.__parameters[name] = Parameter(name=name, meta=self.metadata, global_dims=self.__dimensions, verbose=self.verbose) def add_metadata(self, name: str, @@ -501,15 +521,15 @@ def check(self): # pragma: no cover valid_max = pp.meta['maximum'] default_val = pp.meta['default'] - if not (isinstance(valid_min, str) or isinstance(valid_max, str)): - con.print(f' [dark_orange]WARNING[/]: Value(s) (range: {pp_stats.min}, {pp_stats.max}) outside ' - + f'the valid range of ({valid_min}, {valid_max}); ' - + f'under/over=({pp_outliers.under}, {pp_outliers.over})') - elif valid_min == 'bounded': - # TODO: Handling bounded parameters needs improvement - con.print(f' [dark_orange]WARNING[/]: Bounded parameter value(s) ' - + f'(range: {pp_stats.min}, {pp_stats.max}) outside ' - + f'the valid range of ({default_val}, {valid_max})') + # if not (isinstance(valid_min, str) or isinstance(valid_max, str)): + con.print(f' [dark_orange]WARNING[/]: Value(s) (range: {pp_stats.min}, {pp_stats.max}) outside ' + + f'the valid range of ({valid_min}, {valid_max}); ' + + f'under/over=({pp_outliers.under}, {pp_outliers.over})') + # elif valid_min == 'bounded': + # # TODO: Handling bounded parameters needs improvement + # con.print(f' [dark_orange]WARNING[/]: Bounded parameter value(s) ' + # + f'(range: {pp_stats.min}, {pp_stats.max}) outside ' + # + f'the valid range of ({default_val}, {valid_max})') dims = list(pp.dimensions.keys()) From 05e736a2e45f479a8818751537381cb3c27f2abc Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 06:58:32 -0600 Subject: [PATCH 03/12] Update tests for bounded parameter handling changes - Add test class ordering in conftest.py via pytest_collection_modifyitems - Add tests for bounded parameter creation with and without global_dims - Replace basin_solsta (bounded) with non-bounded params (ncol, maxiter_sntemp, gwstor_init) in tests that don't require bounded behavior - Use separate MetaData copies in test_ParameterFile to avoid shared state - Update expected output strings for new dimension/parameter additions --- tests/conftest.py | 22 ++++++++++ tests/func/test_Parameter.py | 75 ++++++++++++++++++++------------ tests/func/test_ParameterFile.py | 6 ++- tests/func/test_Parameters.py | 10 ++++- 4 files changed, 82 insertions(+), 31 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 118e433..a0d6035 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,28 @@ from shutil import copytree from pathlib import Path +def pytest_collection_modifyitems(items): + """Modifies test items in place to ensure test classes run in a given order.""" + CLASS_ORDER = ['TestPrmsHelpers', + 'TestMetaData', + 'TestDimension', 'TestEmptyDimensions', 'TestEmptyParamDimensions', + 'TestControlVariable', 'TestControl', 'TestControlFile', + 'TestParameter', 'TestParameters', 'TestParameterFile', 'TestParamDb', 'TestParameterNetCDF', + 'TestOutputVariables', 'TestOutputCSV', 'TestOutputCSVFileNotFound', 'TestOutputVariableFileNotFound', + 'TestDataFile', + 'TestCbh'] + sorted_items = items.copy() + + # read the class names from default items + class_mapping = {item: item.cls.__name__ for item in items} + + # Iteratively move tests of each class to the end of the test queue + for class_ in CLASS_ORDER: + sorted_items = ([it for it in sorted_items if class_mapping[it] != class_] + + [it for it in sorted_items if class_mapping[it] == class_]) + + + items[:] = sorted_items @pytest.fixture def datadir(tmp_path, request, scope='function'): diff --git a/tests/func/test_Parameter.py b/tests/func/test_Parameter.py index 86e2dec..52f99f0 100644 --- a/tests/func/test_Parameter.py +++ b/tests/func/test_Parameter.py @@ -4,10 +4,11 @@ import xml.dom.minidom as minidom import xml.etree.ElementTree as xmlET +from pyPRMS import Dimension from pyPRMS import Dimensions from pyPRMS import Parameter from pyPRMS import MetaData -from pyPRMS.Exceptions_custom import FixedDimensionError +from pyPRMS.Exceptions_custom import FixedDimensionError, ParameterNotValidError # @pytest.fixture(scope='class') # def dims_obj(): @@ -28,14 +29,28 @@ class TestParameter: # - modified flag set when data is changed @pytest.mark.parametrize('name, ndim', [('cov_type', 1), - ('tmax_adj', 2), - ('basin_solsta', 0)]) + ('tmax_adj', 2)]) def test_create_parameter(self, metadata_instance, name, ndim): # prms_meta = MetaData(verbose=False).metadata['parameters'] aparam = Parameter(name=name, meta=metadata_instance) assert aparam.name == name and aparam.ndim == ndim + @pytest.mark.parametrize('name, ndim, bname', [('basin_solsta', 0, 'nsol')]) + def test_create_bounded_parameter(self, metadata_instance, name, ndim, bname): + global_dimensions = Dimensions(metadata=MetaData(verbose=False).metadata) + for xx in metadata_instance[name]['dimensions']: + global_dimensions.add(name=xx) + global_dimensions.add(name=bname) + + aparam = Parameter(name=name, meta=metadata_instance, global_dims=global_dimensions) + assert aparam.name == name and aparam.ndim == ndim + + @pytest.mark.parametrize('name, ndim, bname', [('basin_solsta', 0, 'nsol')]) + def test_create_bounded_parameter_nodims(self, metadata_instance, name, ndim, bname): + with pytest.raises(ParameterNotValidError): + aparam = Parameter(name=name, meta=metadata_instance) + def test_create_parameter_bad(self, metadata_instance): """Add parameter which does not exist in metadata""" with pytest.raises(ValueError): @@ -77,15 +92,13 @@ def test_create_parameter_no_metadata_nostrict(self): assert not aparam.is_poi_param() @pytest.mark.parametrize('name, isscalar', [('cov_type', False), - ('tmax_adj', False), - ('basin_solsta', True)]) + ('tmax_adj', False)]) def test_is_scalar(self, metadata_instance, name, isscalar): aparam = Parameter(name=name, meta=metadata_instance) assert aparam.name == name and aparam.is_scalar == isscalar @pytest.mark.parametrize('name, data', [('cov_type', np.array([1, 0, 1, 2], dtype=np.int32)), - ('tmax_adj', np.array([[2.0, 1.2, 3.3, 0], [2.2, 8, 4, 9]], dtype=np.float32)), - ('basin_solsta', np.int32(8))]) + ('tmax_adj', np.array([[2.0, 1.2, 3.3, 0], [2.2, 8, 4, 9]], dtype=np.float32))]) def test_new_param_data(self, metadata_instance, name, data): aparam = Parameter(name=name, meta=metadata_instance) aparam.data = data @@ -100,17 +113,16 @@ def test_new_param_no_data(self, metadata_instance): _ = aparam.data @pytest.mark.parametrize('name, data, expected', [('cov_type', np.array([1.4, 0, 1.6, 2.1], dtype=np.float32), np.array([1, 0, 1, 2], dtype=np.int32)), - ('tmax_adj', np.array([[2, 1, 3, 0], [2, 8, 4, 9]], dtype=np.int32), np.array([[2.0, 1.0, 3.0, 0], [2.0, 8, 4, 9]], dtype=np.float32)), - ('basin_solsta', np.float32(8.2), np.int32(8))]) + ('tmax_adj', np.array([[2, 1, 3, 0], [2, 8, 4, 9]], dtype=np.int32), np.array([[2.0, 1.0, 3.0, 0], [2.0, 8, 4, 9]], dtype=np.float32))]) def test_new_param_data_cast(self, metadata_instance, name, data, expected): aparam = Parameter(name=name, meta=metadata_instance) aparam.data = data assert (aparam.data == expected).all() - @pytest.mark.parametrize('name, data, expected', [('basin_solsta', np.array([2], dtype=np.int32), 2), - ('basin_solsta', np.array([3.0], dtype=np.float32), 3), - ('basin_solsta', np.array([2.5], dtype=np.float32), 2)]) + @pytest.mark.parametrize('name, data, expected', [('maxiter_sntemp', np.array([10], dtype=np.int32), 10), + ('maxiter_sntemp', np.array([12.0], dtype=np.float32), 12), + ('maxiter_sntemp', np.array([20.5], dtype=np.float32), 20)]) def test_new_param_data_scalar_given_array(self, metadata_instance, name, data, expected): aparam = Parameter(name=name, meta=metadata_instance) @@ -118,10 +130,10 @@ def test_new_param_data_scalar_given_array(self, metadata_instance, name, data, assert aparam.data == expected def test_new_param_data_scalar_given_array_too_big(self, metadata_instance): - aparam = Parameter(name='basin_solsta', meta=metadata_instance) + aparam = Parameter(name='maxiter_sntemp', meta=metadata_instance) with pytest.raises(IndexError): - aparam.data = np.array([1, 2], dtype=np.int32) + aparam.data = np.array([10, 20], dtype=np.int32) @pytest.mark.parametrize('name, data', [('cov_type', np.array([1], dtype=np.int32)), ('tmax_adj', np.array([2.3], dtype=np.float32)), @@ -144,12 +156,12 @@ def test_new_param_data_expand(self, metadata_instance, name, data): assert aparam.data.size == expected_size - @pytest.mark.parametrize('name, new_vals', [('basin_solsta', 4)]) + @pytest.mark.parametrize('name, new_vals', [('gwstor_init', 4.0)]) def test_param_update_element_nodata(self, metadata_instance, name, new_vals): """Test updating parameter element""" global_dimensions = Dimensions(metadata=MetaData(verbose=False).metadata) global_dimensions.add(name='nhru', size=2) - global_dimensions.add(name='one', size=1) + # global_dimensions.add(name='one', size=1) aparam = Parameter(name=name, meta=metadata_instance, global_dims=global_dimensions) assert aparam.modified is False @@ -157,13 +169,13 @@ def test_param_update_element_nodata(self, metadata_instance, name, new_vals): with pytest.raises(TypeError): aparam.update_element(index=1, value=new_vals) - @pytest.mark.parametrize('name, data, new_vals', [('basin_solsta', np.array([2], dtype=np.int32), 4), - ('basin_solsta', np.array([2], dtype=np.int32), np.array([4], dtype=np.int32)), - ('basin_solsta', np.array([2], dtype=np.int32), [3])]) + @pytest.mark.parametrize('name, data, new_vals', [('ncol', np.array([2], dtype=np.int32), 4), + ('ncol', np.array([2], dtype=np.int32), np.array([4], dtype=np.int32)), + ('ncol', np.array([2], dtype=np.int32), [3])]) def test_param_update_element_scalar(self, metadata_instance, name, data, new_vals): """Test updating parameter element""" global_dimensions = Dimensions(metadata=MetaData(verbose=False).metadata) - global_dimensions.add(name='nhru', size=2) + # global_dimensions.add(name='nhru', size=2) global_dimensions.add(name='one', size=1) aparam = Parameter(name=name, meta=metadata_instance, global_dims=global_dimensions) @@ -175,8 +187,8 @@ def test_param_update_element_scalar(self, metadata_instance, name, data, new_va assert not (aparam.data_raw == data).all() assert (aparam.data_raw == new_vals).all() - @pytest.mark.parametrize('name, data, new_vals', [('basin_solsta', np.array([2], dtype=np.int32), np.array([2, 3], dtype=np.int32)), - ('basin_solsta', np.array([2], dtype=np.int32), [4, 5])]) + @pytest.mark.parametrize('name, data, new_vals', [('ncol', np.array([2], dtype=np.int32), np.array([2, 3], dtype=np.int32)), + ('ncol', np.array([2], dtype=np.int32), [4, 5])]) def test_param_update_element_scalar_type_error(self, metadata_instance, name, data, new_vals): """Test updating parameter element""" global_dimensions = Dimensions(metadata=MetaData(verbose=False).metadata) @@ -297,7 +309,7 @@ def test_param_subset_by_index(self, metadata_instance, name, data, expected): [2.0, 2.1, 2.2, 2.3, 2.5, 2.6, 2.7, 2.8, 2.0, 2.1, 2.2, 2.3], [3.0, 3.1, 3.2, 3.3, 3.5, 3.6, 3.7, 3.8, 3.0, 3.1, 3.2, 3.3]], dtype=np.float32), 'nmonths'), - ('basin_solsta', + ('ncol', np.int32(2), 'one'),]) def test_param_subset_by_index_fixed_dim(self, metadata_instance, name, data, dim ): @@ -353,7 +365,7 @@ def test_param_change_data_wrong_shape(self, metadata_instance, name, data, new_ aparam.data = data aparam.data = new_data - @pytest.mark.parametrize('name, data, new_data', [('basin_solsta', + @pytest.mark.parametrize('name, data, new_data', [('ncol', np.int32(8), np.array([1, 0, 1, 2], dtype=np.int32))]) def test_param_scalar_data_wrong_class(self, metadata_instance, name, data, new_data): @@ -399,9 +411,14 @@ def test_new_param_all_values_not_equal(self, metadata_instance, name, data): ('poi_type', False, False, True), ('seg_elev', False, True, False), ('seg_humidity', False, True, False), - ('basin_solsta', False, False, False)]) + ('ncol', False, False, False)]) def test_param_check_dim_type(self, metadata_instance, name, ishru, isseg, ispoi): - aparam = Parameter(name=name, meta=metadata_instance) + global_dimensions = Dimensions(metadata=MetaData(verbose=False).metadata) + for xx in metadata_instance[name]['dimensions']: + global_dimensions.add(name=xx) + global_dimensions.add(name='nsegment') + + aparam = Parameter(name=name, meta=metadata_instance, global_dims=global_dimensions) assert (aparam.is_hru_param() == ishru and aparam.is_seg_param() == isseg and aparam.is_poi_param() == ispoi) @@ -521,7 +538,7 @@ def test_remove_by_index_4(self, metadata_instance, name, dim): @pytest.mark.parametrize('name, data, expected', [('cov_type', np.array([1, 2, 3], dtype=np.int32), np.array([2, 2, 1, 3], dtype=np.int32)), - ('basin_solsta', + ('ncol', np.int32(8), np.array([8, 8, 8, 8], dtype=np.int32))]) def test_stats(self, metadata_instance, name, data, expected): @@ -559,9 +576,9 @@ def test_stats_string(self, metadata_instance, name, data): ('hru_slope', np.array([0.082, 0.106, 0.069, 0.073], dtype=np.float32), '$id,hru_slope\n1,0.082\n2,0.106\n3,0.069\n4,0.073\n'), - ('basin_solsta', + ('ncol', np.int32(8), - '$id,basin_solsta\n1,8\n')]) + '$id,ncol\n1,8\n')]) def test_toparamdb(self, metadata_instance, name, data, expected): """Test the toparamdb method""" aparam = Parameter(name=name, meta=metadata_instance) diff --git a/tests/func/test_ParameterFile.py b/tests/func/test_ParameterFile.py index ffab7ab..1cce67e 100644 --- a/tests/func/test_ParameterFile.py +++ b/tests/func/test_ParameterFile.py @@ -224,9 +224,13 @@ def test_diff_parameters(self, datadir): parameter_file = datadir / 'myparam.param' prms_meta = MetaData(verbose=False).metadata + # We need two independent copies of the metadata since it is modified in-place + # when the parameter file is read. + prms_meta_2 = MetaData(verbose=False).metadata.copy() + ctl = ControlFile(control_file, metadata=prms_meta, verbose=False) pdb_orig = ParameterFile(parameter_file, metadata=prms_meta) - pdb = ParameterFile(parameter_file, metadata=prms_meta) + pdb = ParameterFile(parameter_file, metadata=prms_meta_2) pdb.control = ctl expected_diff = {'self_not_other': set(), 'other_not_self': {'pref_flow_infil_frac'}, 'diffs': {}} diff --git a/tests/func/test_Parameters.py b/tests/func/test_Parameters.py index 99068c6..5d0cb1f 100644 --- a/tests/func/test_Parameters.py +++ b/tests/func/test_Parameters.py @@ -51,19 +51,27 @@ def test_xml_global_parameters(self, pdb_instance): assert xmlET.tostring(xml) == expected_xml + def test_add_hru_deplcrv_with_no_ndeplval(self, pdb_instance): + with pytest.raises(ValueError): + pdb_instance.add(name='hru_deplcrv') + @pytest.mark.parametrize('name', [('cov_type'), ('tmin_cbh_adj'), ('tmax_adj'), ('basin_solsta'), + ('hru_deplcrv'), ('poi_gage_id'), ('poi_gage_segment'), ('poi_type')]) def test_add_valid_parameter(self, pdb_instance, name): + # Add ndeplval dimension so adding hru_deplcrv will succeed + pdb_instance.dimensions.add(name='ndeplval', size=11) + pdb_instance.add(name=name) assert pdb_instance.exists(name=name) def test_parameters_str(self, pdb_instance): - expected = '----- Dimensions -----\nnhru: size=4\nnmonths: size=12\none: size=1\nnpoigages: size=4\nnobs: size=4\n----- Parameters -----\ntmax_cbh_adj [nhru, nmonths]\ncov_type [nhru]\ntmin_cbh_adj [nhru, nmonths]\ntmax_adj [nhru, nmonths]\nbasin_solsta [one]\npoi_gage_id [npoigages]\npoi_gage_segment [npoigages]\npoi_type [npoigages]\n' + expected = '----- Dimensions -----\nnhru: size=4\nnmonths: size=12\none: size=1\nnpoigages: size=4\nnobs: size=4\nndeplval: size=11\nnsol: size=0\nndepl: size=1\nnsegment: size=0\n----- Parameters -----\ntmax_cbh_adj [nhru, nmonths]\ncov_type [nhru]\ntmin_cbh_adj [nhru, nmonths]\ntmax_adj [nhru, nmonths]\nbasin_solsta [one]\nhru_deplcrv [nhru]\npoi_gage_id [npoigages]\npoi_gage_segment [npoigages]\npoi_type [npoigages]\n' assert pdb_instance.__str__() == expected From 63f0ae0c26256a3c492c4537e6e673ca62d973e8 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 06:58:42 -0600 Subject: [PATCH 04/12] Fix Cbh and DataFile issues - Auto-set nhm_id from parameters when available in Cbh constructor - Fix cbh_files dict key to use string path instead of Path object - Fix resolve_units docstring formatting - Pass parameters to Cbh in netcdf test - Rename TestStreamflow class to TestDataFile in test_DataFile.py --- pyPRMS/cbh/Cbh.py | 10 +++++++--- tests/func/test_Cbh.py | 6 +++--- tests/func/test_DataFile.py | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pyPRMS/cbh/Cbh.py b/pyPRMS/cbh/Cbh.py index 90fda5c..679fa6f 100644 --- a/pyPRMS/cbh/Cbh.py +++ b/pyPRMS/cbh/Cbh.py @@ -47,7 +47,8 @@ def __init__(self, src_path: str | Path | list[str | Path], :param src_path: List of paths to CBH files :param metadata: Metadata dictionary for Climate-by-HRU variables :param engine: Engine to use for reading CBH files (one of netcdf, zarr, or ascii) - :param control: Control object for PRMS model containing configuration information + :param control: Control object for PRMS model containing configuration information; only needed when reading ASCII CBH files + :param parameters: Parameters object containing model parameters :param verbose: Output debugging information """ @@ -85,6 +86,9 @@ def __init__(self, src_path: str | Path | list[str | Path], self.__dataset = ds + if self.__parameters is not None: + self.set_nhm_id(self.__parameters.get('nhm_id').data) + def __repr__(self) -> str: """String representation of the Cbh object. @@ -115,7 +119,7 @@ def cbh_src(self) -> dict[str, str]: return self.__cbh_src def resolve_units(self): - """Adjust units metadata for CBH variables that have an initial units value of + """Adjust `units` metadata for CBH variables that have an initial units value of precip_units or temp_units. :returns: None @@ -362,7 +366,7 @@ def _read_ascii(self, control: Control | None) -> xr.Dataset: if (self.__src_path[0] / cfile).exists(): if self.verbose: con.print(f'[green]INFO[/]: Found {cfile}') - cbh_files[self.__src_path[0] / cfile] = prms_var + cbh_files[str(self.__src_path[0] / cfile)] = prms_var return self._cbh_to_xarray(cbh_files) diff --git a/tests/func/test_Cbh.py b/tests/func/test_Cbh.py index b10165b..b6ef152 100644 --- a/tests/func/test_Cbh.py +++ b/tests/func/test_Cbh.py @@ -59,8 +59,8 @@ def test_read_ctl_ascii_roundtrip_ascii(self, datadir, pdb_instance, meta_instan cbh = Cbh(str(datadir), engine='ascii', metadata=meta_instance.metadata, control=ctl, parameters=pdb_instance, verbose=True) - assert not cbh.has_nhm_id - cbh.set_nhm_id(nhm_ids) + # assert not cbh.has_nhm_id + # cbh.set_nhm_id(nhm_ids) assert cbh.has_nhm_id for cvar in cbh.data.data_vars: @@ -100,7 +100,7 @@ def test_read_netcdf_roundtrip_netcdf(self, datadir, pdb_instance, meta_instance out_path = tmp_path / 'run_files' out_path.mkdir() - cbh = Cbh(str(datadir.join('cbh.nc')), engine='netcdf', metadata=meta_instance.metadata) + cbh = Cbh(str(datadir.join('cbh.nc')), engine='netcdf', metadata=meta_instance.metadata, parameters=pdb_instance) out_file = out_path / 'cbh.nc' cbh.write_netcdf(out_file) diff --git a/tests/func/test_DataFile.py b/tests/func/test_DataFile.py index a6e2218..1b6cd08 100644 --- a/tests/func/test_DataFile.py +++ b/tests/func/test_DataFile.py @@ -28,7 +28,7 @@ def datadir(tmpdir, request): return tmpdir -class TestStreamflow: +class TestDataFile: def test_read_datafile_single_station(self, datadir): sf_filename = datadir / 'sf_data_pipestem_bandit' From d8076b6dad2e338df989ec5400d30e38732d1731 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:34:27 -0600 Subject: [PATCH 05/12] Move bounded parameter resolution outside dimension loop The is_bounded check was inside the 'for cname in dimensions' loop, causing it to run on every iteration. For multi-dimension bounded parameters this would fail on the second iteration since maximum is already resolved to a numeric value. Move the resolution to run once after all dimensions are added. --- pyPRMS/parameters/Parameter.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/pyPRMS/parameters/Parameter.py b/pyPRMS/parameters/Parameter.py index ec80adb..7fddfa7 100644 --- a/pyPRMS/parameters/Parameter.py +++ b/pyPRMS/parameters/Parameter.py @@ -75,23 +75,21 @@ def __init__(self, name: str, for cname in self.meta['dimensions']: self.__dimensions.add(cname) - # TODO: 2026-08-12 PAN - if this is a bounded parameter and global_dims - # was not passed then what should happen? an error? a warning? nothing? - # if self.meta.get('maximum') in list(self.dimensions.keys()): - if self.meta.get('is_bounded', False): - if global_dims is None: - raise ParameterNotValidError(f'Parameter, {self.name}, is bounded but no global dimensions were supplied') - - # Save the name of the bounded-dimension - self.meta['bounded_dimension_name'] = self.meta.get('maximum') - self.meta['maximum'] = global_dims.get(self.meta.get('bounded_dimension_name')).size - - if self.__verbose: # pragma: no cover - con.print(f'[bold]{self.name}[/]: valid upper bound adjusted to {self.meta["maximum"]}') - if global_dims is not None: self.__dimensions[cname].size = global_dims.get(cname).size self.__dimensions[cname].meta = global_dims[cname].meta + + # Resolve bounded parameter maximum from dimension name to numeric size + if self.meta.get('is_bounded', False): + if global_dims is None: + raise ParameterNotValidError(f'Parameter, {self.name}, is bounded but no global dimensions were supplied') + + # Save the name of the bounded-dimension + self.meta['bounded_dimension_name'] = self.meta.get('maximum') + self.meta['maximum'] = global_dims.get(self.meta.get('bounded_dimension_name')).size + + if self.__verbose: # pragma: no cover + con.print(f'[bold]{self.name}[/]: valid upper bound adjusted to {self.meta["maximum"]}') else: raise ValueError(f'`{self.name}` does not exist in metadata') else: From 1d520984884c9256a5b129ab04d4a0116243b783 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:35:37 -0600 Subject: [PATCH 06/12] Add deprecation warning to adjust_bounded_parameters() The method is no longer called internally since bounded parameters are now resolved at creation time in Parameter.__init__. Emits a DeprecationWarning to inform any external callers. --- pyPRMS/parameters/Parameters.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pyPRMS/parameters/Parameters.py b/pyPRMS/parameters/Parameters.py index 42e628a..5ec047c 100644 --- a/pyPRMS/parameters/Parameters.py +++ b/pyPRMS/parameters/Parameters.py @@ -10,6 +10,7 @@ import numpy.typing as npt import pandas as pd # type: ignore import sys +import warnings import xml.dom.minidom as minidom import xml.etree.ElementTree as xmlET @@ -456,8 +457,20 @@ def add_poi(self, addl_gages: dict[str, int]): def adjust_bounded_parameters(self): """Adjust the valid upper and lower values for bounded parameters. + + .. deprecated:: + Bounded parameters are now resolved at creation time in + :meth:`Parameter.__init__`. This method will be removed in a + future release. """ + warnings.warn( + 'adjust_bounded_parameters() is deprecated and will be removed in a future release. ' + 'Bounded parameters are now resolved at creation time.', + DeprecationWarning, + stacklevel=2, + ) + for cparam in self.parameters.values(): cmeta = cparam.meta From c926ba59a84a7979edabb443588061f7f994cebd Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:37:25 -0600 Subject: [PATCH 07/12] Add string datatype guard to outlier_ids() Skip comparison for string-type parameters to avoid TypeError when comparing string data against numeric min/max bounds. Matches the guards already present in check_values() and outliers(). --- pyPRMS/parameters/Parameters.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pyPRMS/parameters/Parameters.py b/pyPRMS/parameters/Parameters.py index 5ec047c..8e35aa5 100644 --- a/pyPRMS/parameters/Parameters.py +++ b/pyPRMS/parameters/Parameters.py @@ -743,6 +743,9 @@ def outlier_ids(self, name: str) -> list[int]: cparam = self.get(name) + if cparam.meta.get('datatype') == 'string': + return [] + param_data = self.get_dataframe(name) bad_value_ids = param_data[(param_data[name] < cparam.meta['minimum']) | (param_data[name] > cparam.meta['maximum'])].index.tolist() From 9bec0c9084dcbcc4b0de52feadbb2466088cadf9 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:38:59 -0600 Subject: [PATCH 08/12] Remove stale commented-out bounded parameter code Clean up leftover comments from the old bounded handling approach in check() and check_values(). Remove unused default_val variable. --- pyPRMS/parameters/Parameter.py | 1 - pyPRMS/parameters/Parameters.py | 7 ------- 2 files changed, 8 deletions(-) diff --git a/pyPRMS/parameters/Parameter.py b/pyPRMS/parameters/Parameter.py index 7fddfa7..e6510e5 100644 --- a/pyPRMS/parameters/Parameter.py +++ b/pyPRMS/parameters/Parameter.py @@ -368,7 +368,6 @@ def check_values(self) -> bool: if self.meta.get('datatype') != 'string': if minval is not None and maxval is not None: # Check both ends of the range - # if not (isinstance(minval, str) or isinstance(maxval, str)): return (self.data_raw >= minval).all() and (self.data_raw <= maxval).all().item() return True diff --git a/pyPRMS/parameters/Parameters.py b/pyPRMS/parameters/Parameters.py index 8e35aa5..e4f3e7e 100644 --- a/pyPRMS/parameters/Parameters.py +++ b/pyPRMS/parameters/Parameters.py @@ -532,17 +532,10 @@ def check(self): # pragma: no cover pp_outliers = pp.outliers() valid_min = pp.meta['minimum'] valid_max = pp.meta['maximum'] - default_val = pp.meta['default'] - # if not (isinstance(valid_min, str) or isinstance(valid_max, str)): con.print(f' [dark_orange]WARNING[/]: Value(s) (range: {pp_stats.min}, {pp_stats.max}) outside ' + f'the valid range of ({valid_min}, {valid_max}); ' + f'under/over=({pp_outliers.under}, {pp_outliers.over})') - # elif valid_min == 'bounded': - # # TODO: Handling bounded parameters needs improvement - # con.print(f' [dark_orange]WARNING[/]: Bounded parameter value(s) ' - # + f'(range: {pp_stats.min}, {pp_stats.max}) outside ' - # + f'the valid range of ({default_val}, {valid_max})') dims = list(pp.dimensions.keys()) From 94e5459805355e98fcf918ed31680ea1f980c549 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:46:13 -0600 Subject: [PATCH 09/12] Fall back to metadata default for nobs when npoigages is missing Previously, adding a bounded parameter that requires nobs would crash if npoigages was also missing from global dimensions. Now falls back to adding nobs with its metadata default size (0). --- pyPRMS/parameters/Parameters.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pyPRMS/parameters/Parameters.py b/pyPRMS/parameters/Parameters.py index e4f3e7e..bfcebe1 100644 --- a/pyPRMS/parameters/Parameters.py +++ b/pyPRMS/parameters/Parameters.py @@ -357,8 +357,12 @@ def add(self, name: str): # This is the one dimension where the size depends on another dimension self.dimensions.add(name=bounded_dim_name, size=int(self.dimensions.get('ndeplval').size / 11)) elif bounded_dim_name == 'nobs': - # If this is missing it should be added with the same value as npoigages. - self.dimensions.add(name=bounded_dim_name, size=self.dimensions.get('npoigages').size) + # If this is missing it should be added with the same value as npoigages + # or the metadata default if npoigages is also missing. + if self.__dimensions.exists('npoigages'): + self.dimensions.add(name=bounded_dim_name, size=self.dimensions.get('npoigages').size) + else: + self.__dimensions.add(bounded_dim_name) else: self.__dimensions.add(bounded_dim_name) From 2297da9e0b979834713d5ca4aaa38394b9c68d48 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:51:26 -0600 Subject: [PATCH 10/12] Add min/max validation to Parameter.outliers() Raise ValueError if minimum > maximum (indicates corrupted metadata). Emit a warning when minimum == maximum (dimension size is zero, likely an unpopulated dimension). --- pyPRMS/parameters/Parameter.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/pyPRMS/parameters/Parameter.py b/pyPRMS/parameters/Parameter.py index e6510e5..eb5f74c 100644 --- a/pyPRMS/parameters/Parameter.py +++ b/pyPRMS/parameters/Parameter.py @@ -412,17 +412,28 @@ def outliers(self) -> Outliers: """Returns the number of values less than or greater than the valid range :returns: NamedTuple containing count of values less than and values greater than valid range + :raises ValueError: If minimum is greater than maximum """ values_under = 0 values_over = 0 if self.meta.get('datatype') != 'string': - if self.meta.get('minimum', None) is not None: - values_under = np.count_nonzero(self.data_raw < self.meta.get('minimum')) # type: ignore + minval = self.meta.get('minimum', None) + maxval = self.meta.get('maximum', None) - if self.meta.get('maximum', None) is not None: - values_over = np.count_nonzero(self.data_raw > self.meta.get('maximum')) # type: ignore + if minval is not None and maxval is not None: + if minval > maxval: + raise ValueError(f'{self.name}: minimum ({minval}) is greater than maximum ({maxval})') + + if minval == maxval: + con.print(f'[orange3]WARNING[/]: {self.name}: minimum and maximum are both {minval}') + + if minval is not None: + values_under = np.count_nonzero(self.data_raw < minval) # type: ignore + + if maxval is not None: + values_over = np.count_nonzero(self.data_raw > maxval) # type: ignore return Outliers(self.__name, values_under, values_over) From 19d752b994a04257b9e080e9eb9537b6597cb72b Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 07:53:40 -0600 Subject: [PATCH 11/12] Guard outlier_ids() against None min/max metadata Report partial outliers when only one bound is available rather than returning an empty list. Emit warnings when minimum, maximum, or both are undefined so the caller knows the check was incomplete. --- pyPRMS/parameters/Parameters.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/pyPRMS/parameters/Parameters.py b/pyPRMS/parameters/Parameters.py index bfcebe1..4747d3c 100644 --- a/pyPRMS/parameters/Parameters.py +++ b/pyPRMS/parameters/Parameters.py @@ -743,9 +743,28 @@ def outlier_ids(self, name: str) -> list[int]: if cparam.meta.get('datatype') == 'string': return [] + minval = cparam.meta.get('minimum', None) + maxval = cparam.meta.get('maximum', None) + + if minval is None and maxval is None: + con.print(f'[orange3]WARNING[/]: {name}: both minimum and maximum are undefined; cannot determine outliers') + return [] + + if minval is None: + con.print(f'[orange3]WARNING[/]: {name}: minimum is undefined; only checking maximum bound') + elif maxval is None: + con.print(f'[orange3]WARNING[/]: {name}: maximum is undefined; only checking minimum bound') + param_data = self.get_dataframe(name) - bad_value_ids = param_data[(param_data[name] < cparam.meta['minimum']) | - (param_data[name] > cparam.meta['maximum'])].index.tolist() + + conditions = [] + if minval is not None: + conditions.append(param_data[name] < minval) + if maxval is not None: + conditions.append(param_data[name] > maxval) + + mask = conditions[0] if len(conditions) == 1 else (conditions[0] | conditions[1]) + bad_value_ids = param_data[mask].index.tolist() return bad_value_ids From 24678ad202ae3ed4c11eed33e9e3152c1b610215 Mon Sep 17 00:00:00 2001 From: Parker Norton Date: Fri, 14 Aug 2026 08:07:44 -0600 Subject: [PATCH 12/12] Add missing trailing newline to ParameterNetCDF.py --- pyPRMS/parameters/ParameterNetCDF.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyPRMS/parameters/ParameterNetCDF.py b/pyPRMS/parameters/ParameterNetCDF.py index 265a0be..53034b5 100644 --- a/pyPRMS/parameters/ParameterNetCDF.py +++ b/pyPRMS/parameters/ParameterNetCDF.py @@ -76,4 +76,4 @@ def _read(self): self.add(name=str(var)) # Add the data - self.get(str(var)).data = cparam.values \ No newline at end of file + self.get(str(var)).data = cparam.values