adjustments to support gridded forcing#202
Conversation
| @property | ||
| def cat_ids(self) -> NDArray[np.int_]: | ||
| """Get the catchment IDs for the hydrofabric domain.""" | ||
| if self._mpi_meta.rank == 0: | ||
| cat_ids = esmf_creation.create_mesh(self._job_meta) | ||
| cat_count = np.array( | ||
| [len(cat_ids) if self._mpi_meta.rank == 0 else 0], dtype=np.intc | ||
| ) | ||
| self._mpi_meta.comm.Bcast(cat_count, root=0) | ||
| if self._mpi_meta.rank != 0: | ||
| cat_ids = np.empty(cat_count[0], dtype=np.int64) | ||
| self._mpi_meta.comm.Bcast(cat_ids, root=0) | ||
| return cat_ids |
There was a problem hiding this comment.
This feels awkward as a property that non-hydrofabric models have to conform to. Maybe change this to a private method that gets overwritten.
| @property | |
| def cat_ids(self) -> NDArray[np.int_]: | |
| """Get the catchment IDs for the hydrofabric domain.""" | |
| if self._mpi_meta.rank == 0: | |
| cat_ids = esmf_creation.create_mesh(self._job_meta) | |
| cat_count = np.array( | |
| [len(cat_ids) if self._mpi_meta.rank == 0 else 0], dtype=np.intc | |
| ) | |
| self._mpi_meta.comm.Bcast(cat_count, root=0) | |
| if self._mpi_meta.rank != 0: | |
| cat_ids = np.empty(cat_count[0], dtype=np.int64) | |
| self._mpi_meta.comm.Bcast(cat_ids, root=0) | |
| return cat_ids | |
| def _get_cat_ids(self) -> NDArray[np.int64]: | |
| """Get the catchment IDs for the hydrofabric domain.""" | |
| if self._mpi_meta.rank == 0: | |
| cat_ids = esmf_creation.create_mesh(self._job_meta) | |
| cat_count = np.array( | |
| [len(cat_ids) if self._mpi_meta.rank == 0 else 0], dtype=np.intc | |
| ) | |
| self._mpi_meta.comm.Bcast(cat_count, root=0) | |
| if self._mpi_meta.rank != 0: | |
| cat_ids = np.empty(cat_count[0], dtype=np.int64) | |
| self._mpi_meta.comm.Bcast(cat_ids, root=0) | |
| return cat_ids |
| if self._mpi_meta.rank != 0: | ||
| cat_ids = np.empty(cat_count[0], dtype=np.int64) | ||
| self._mpi_meta.comm.Bcast(cat_ids, root=0) | ||
| self._values["CAT-ID"] = self.cat_ids |
There was a problem hiding this comment.
| self._values["CAT-ID"] = self.cat_ids | |
| self._values["CAT-ID"] = self._get_cat_ids() |
| z[:] = grid.grid_z | ||
| return z | ||
| raise ValueError(f"get_grid_z: grid_id {grid_id} unknown") | ||
|
|
There was a problem hiding this comment.
| def _get_cat_ids(self) -> None: | |
| """Get the catchment IDs for the hydrofabric domain. Returns `None` for all other models.""" |
| """ | ||
| super().__init__() | ||
| self.GeoMeta = UnstructuredGeoMeta | ||
| self.cat_ids = None |
| """ | ||
| super().__init__() | ||
| self.GeoMeta = GriddedGeoMeta | ||
| self.cat_ids = None |
mxkpp
left a comment
mxkpp
left a comment
There was a problem hiding this comment.
conftest.py still references BMIMODEL which no longer exists.
| self._job_meta, | ||
| self.geo_meta, | ||
| self._mpi_meta, | ||
| self._values.get("CAT-ID"), |
There was a problem hiding this comment.
Is there a reason for .get (returns None when key doesn't exist) instead of bracket notation for accessing this value (raises KeyError).
| hyfab = gpd.read_file(hyfab_name, layer='divides') | ||
| return np.sort(hyfab.div_id.values, copy=True, dtype=np.int64) | ||
| hyfab = gpd.read_file(hyfab_name, layer="divides") | ||
| return np.array(np.sort(hyfab.div_id.values), copy=True,dtype=np.int64) |
There was a problem hiding this comment.
The old code converted to int and then sorted. The pending code sorts the raw values before converting to int. If the raw values are floats, the ordering within each integer range might be different before and after this change. I don't know if that matters or not, or the type of the raw div_id.values, but wanted to mention in case relevant.
Generally, this np.sort() and the usage of copy=True is memory-heavy. If there are memory concerns, places like this could be considered for later changes, modifying arrays in-place rather than creating full copies of them (as long as it is safe to do so, which I did not evalute).
| @cached_property | ||
| def geogrid_ds(self) -> xr.Dataset: | ||
| """Open the geogrid file and return the xarray dataset object.""" | ||
| try: | ||
| with xr.open_dataset(self.config_options.geogrid) as ds: | ||
| return ds.load() | ||
| return ds | ||
| except Exception as e: | ||
| self.config_options.errMsg = "Unable to open geogrid file with xarray" | ||
| log_critical(self.config_options, self.mpi_config) | ||
| raise e | ||
|
|
||
| @cached_property | ||
| @set_none | ||
| def esmf_ds(self) -> xr.Dataset: | ||
| """Open the geospatial metadata file and return the xarray dataset object.""" | ||
| try: | ||
| with xr.open_dataset(self.config_options.spatial_meta) as ds: | ||
| esmf_ds = ds.load() | ||
| esmf_ds = ds | ||
| except Exception as e: | ||
| self.config_options.errMsg = ( | ||
| f"Unable to open esmf file: {self.config_options.spatial_meta}" |
There was a problem hiding this comment.
I didn't try running this, but I think with the removal of .load(), the file handle will be closed by the context manager before the data can be accessed.
3 participants
Modify the code base to better support
griddedmode instead of onlyhydrofabricmode. This will be needed down the road for coastal modeling.I think this resolves the issues discovered in:
Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Target Environment support