Fix handling of bounded parameters

.gitignore

@@ -70,3 +70,6 @@ Icon
 *.swp
 
 .idea
+
+.kiro
+.vscode

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)
 

pyPRMS/metadata/metadata.py

@@ -298,6 +298,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:

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()

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
 
@@ -78,10 +78,22 @@ def __init__(self, name: str,
                         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:
-                # 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 +365,10 @@ 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
                 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
 
@@ -402,16 +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('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':
+            minval = self.meta.get('minimum', None)
+            maxval = self.meta.get('maximum', None)
+
+            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 self.meta.get('maximum', None) is not None:
-            values_over = np.count_nonzero(self.data_raw > self.meta.get('maximum'))   # 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)
 

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

pyPRMS/parameters/ParameterNetCDF.py

@@ -77,5 +77,3 @@ def _read(self):
 
             # Add the data
             self.get(str(var)).data = cparam.values
-
-        self.adjust_bounded_parameters()

pyPRMS/parameters/Parameters.py

@@ -10,9 +10,11 @@
 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
 
+from copy import deepcopy
 from collections import defaultdict
 from collections.abc import KeysView, Sequence
 from functools import cached_property
@@ -66,7 +68,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 +82,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 +335,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 +349,27 @@ 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
+                    # 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)
+
+                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,
@@ -436,8 +461,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
 
@@ -499,17 +536,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})')
+
+                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})')
 
             dims = list(pp.dimensions.keys())
 
@@ -710,9 +740,31 @@ def outlier_ids(self, name: str) -> list[int]:
 
         cparam = self.get(name)
 
+        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
 

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'):

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)

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'