Issue #963 dump meta swap model nc#1852
Conversation
…in utilities, accepting the IModel interface as argument. The MetaSWAP model now also implements this interface, so can be prepared to use the same functionality
JoerivanEngelen
left a comment
JoerivanEngelen
left a comment
There was a problem hiding this comment.
Thanks, the changeset is quite complete and the roundtrip tests are good.
I have some comments though:
-
There is duplicate code in imod.mf6.Model.dump, which is never called now because imod.mf6.Modflow6Simulation doesn't call it anymore... See my detailed comments.
-
It's very useful you added the iModel interface to MetaSwapModel. You had to add some methods to comply with the iModel interface. Unfortunately the masking methods do not work as some important methods are missing. Implementing that in this PR is out of scope, but could you add a NotImplementedError here? Furthermore could you add unittests for the others that we still think work? If it turns out in the unittests that they do not work, also replace these with a NotImplmentedError. List of methods/properties that still deserve a unittest:
- _is_splitting_supported
- _is_regridding_supported
- _is_clipping_supported
- model_id
- options
- domain
|
|
||
|
|
||
| @standard_log_decorator() | ||
| def _dump_model( |
There was a problem hiding this comment.
Great that you are refactoring this to move dumping logic to a utility. Stylewise: could you rename this function to dump_model? The underscore is only used for private methods or private functions WITHIN a module (we have not documented that, so your confusion is very understandable).
| ) | ||
| toml_content[type(pkg).__name__][pkgname] = pkg_path.name | ||
|
|
||
| if hasattr(model, "simulation_settings"): |
There was a problem hiding this comment.
I think this only necessary for MetaSWAP models, right? If so, please add a comment that this is intended for MSW.
There was a problem hiding this comment.
Only applies to MetaSWAP models, I would rather do isinstance and ask if model is a MetaSWAP model, but that introduces a cyclic problem, therefore this indirect approach. I added a comment stating that this line is only relevant for MetaSWAP models.
There was a problem hiding this comment.
You're dumping twice now, if I'm not mistaken, this should be sufficient now:
| toml_path = _dump_model( | |
| self, | |
| modeldirectory, | |
| modelname, | |
| validate=validate, | |
| mdal_compliant=mdal_compliant, | |
| crs=crs, | |
| engine=engine, | |
| ) |
| if isinstance(value, Modflow6Model): | ||
| model_toml_path = value.dump( | ||
| directory, key, validate, mdal_compliant, crs, engine=engine | ||
| model_toml_path = _dump_model( |
There was a problem hiding this comment.
This change is not necessary: calling Modflow6Model.dump (as was done previously) is sufficient here.
| toml_content = tomli.load(f) | ||
|
|
||
| parentdir = toml_path.parent | ||
| if issubclass(cls, MetaSwapModel): |
There was a problem hiding this comment.
This class is never subclassed, so this check is not necessary right?
| def purge_empty_packages( | ||
| self, model_name: Optional[str] = "", ignore_time: bool = False | ||
| ) -> None: | ||
| """ | ||
| This method removes empty packages from the model in place. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| model_name: str, optional | ||
| Name of the model, used for logging. | ||
| ignore_time: bool, optional | ||
| If False, packages are considered empty if they have no data at all | ||
| timesteps. If True, packages are considered empty if they have no | ||
| data at the first time step. The latter can increase performance | ||
| considerably. | ||
| """ | ||
| empty_packages = [ | ||
| package_name | ||
| for package_name, package in self.items() | ||
| if package.is_empty(ignore_time=ignore_time) | ||
| ] | ||
| for package_name in empty_packages: | ||
| self.pop(package_name) |
There was a problem hiding this comment.
I see the imod.msw.MetaSwapPackage class doesn't have an is_empty method, so this code doesn't work unfortunately. Could you replace this code with a NotImplementedError?
| improve performance when masking models with many time steps. | ||
| """ | ||
|
|
||
| mask_all_packages(self, mask, ignore_time_purge_empty) |
There was a problem hiding this comment.
MetaSwap doesn't have masking yet, so could you also change this to a NotImplementedError?
| def _is_scalar_nan(da: GridDataArray): | ||
| """ | ||
| Test if is_scalar_nan, carefully avoid loading grids in memory | ||
| """ | ||
| scalar_data: bool = is_scalar(da) | ||
| if scalar_data: | ||
| stripped_value = da.to_numpy()[()] | ||
| return isinstance(stripped_value, numbers.Real) and np.isnan(stripped_value) # type: ignore[call-overload] | ||
| return False |
There was a problem hiding this comment.
This function is also in imod.mf6.pkgbase.py, could you move this to imod.common.utilities.value_filters?
|
|
||
| def roundtrip(model, tmp_path): | ||
| model.dump(tmp_path, "test") | ||
| _dump_model(model, tmp_path, "test") |
There was a problem hiding this comment.
I think this change is not necessary? See previous comment in imod.mf6.simulation.py
| from imod.util.spatial import empty_2d | ||
|
|
||
|
|
||
| def roundtrip(msw_model, tmpdir_factory, name, engine): |
|
3 participants
Fixes #963
Description
Checklist
Issue #nr, e.g.Issue #737pixi run generate-sbomand committed changes