Small I/O fixes #286
Small I/O fixes #286
Conversation
There was a problem hiding this comment.
Nice that you try to tackle some of the remaining I/O issues! We discussed before but I agree to remove write_config from write_forcing and to write it last in model.write.
For the strings of starttime and endtime, I wonder why did you change? As long as both strings or times can be read by hydromt I do not mind so much.
For the staticgeoms, I suggest two solutions that I would be fine with.
For read and write I would keep the current behavior.
hydromt_wflow/wflow.py
Outdated
| def read( | ||
| self, | ||
| config_fn: Union[Path, str] = None, | ||
| geom_fn: Union[Path, str] = "staticgeoms", |
There was a problem hiding this comment.
| geom_fn: Union[Path, str] = "staticgeoms", | |
| geoms_fn: Union[Path, str] = "staticgeoms", |
hydromt_wflow/wflow.py
Outdated
| def read(self): | ||
| def read( | ||
| self, | ||
| config_fn: Union[Path, str] = None, |
There was a problem hiding this comment.
Do we need this? You can specify the config_fn in the model init or global options in the hydromt model_build.yml config. If we do not then I think we should have something similar for staticgeoms ie defined during model init and keep the current behavior.
hydromt_wflow/wflow.py
Outdated
| def write( | ||
| self, | ||
| config_fn: str = None, | ||
| grid_fn: Union[Path, str] = "staticmaps.nc", | ||
| geom_fn: Union[Path, str] = "staticgeoms", | ||
| forcing_fn: Union[Path, str] = None, | ||
| ): |
There was a problem hiding this comment.
I think if you want to change any default argument of write, you should then call the single write methods one by one. The general write is only meant to be a shortcut for a quick write.
There was a problem hiding this comment.
I know, but I noticed while using it myself that sometimes I want to write everything but change one name of the output. I made it kwargs only to support the quick model.write(). I'd say, it's just a little bit of extra functionality. However I do not feel strongly about this..
| """Write the complete model schematization and configuration to file.""" | ||
| self.logger.info(f"Write model data to {self.root}") | ||
| # if in r, r+ mode, only write updated components | ||
| if not self._write: | ||
| self.logger.warning("Cannot write in read-only mode") | ||
| return | ||
| self.write_data_catalog() | ||
| if self.config: # try to read default if not yet set |
There was a problem hiding this comment.
Most filenames are read from the config and as the comment says, we did find some cases where at that stage config was not yet read. To be sure, I would then leave a call to self.config to be sure it is read. You can keep the writing as last.
| dir_default = join(self.root, "staticmaps.nc") | ||
| dir_mod = dirname( | ||
| self.get_config("input.path_static", abs_path=True, fallback=dir_default) | ||
| ) | ||
| fns = glob.glob(join(dir_mod, geom_fn, "*.geojson")) |
There was a problem hiding this comment.
I think I did that in case staticmaps path was for example ../../staticmaps. Then the user does not have to think too much about the staticgeoms path as you can assume it will most probably be in the same folder as staticmaps. The one thing I did not take into account and that leads to errors is that in the toml you can now specify a dir_input like you have a dir_output. I think it may be nicer to keep the current behavior and to also handle the dir_input for staticgeoms see #276
What do you think?
Else this means the user needs to manually specify the staticgeoms folder and to avoid making it too complex (ie can easily call read/write), I would then put the geom_fn as a init argument of model.
So I would be okay it either 1 we keep the current code and add dir_input or 2) we keep your implementation but geom_fn can be defined in init.
There was a problem hiding this comment.
Well most of the time the staticmaps is in the same folder as the config file, i.e. the root directory, so I felt like that the addition of the staticmaps is a bit too much. Therefore, It made, at least to me, not a lot of sense to rely on the staticmaps. Maybe it's best to have a quick call regarding this.
|
I directly changed the PR @dalmijn, hope it's okay.
All the rest was good, thanks for the changes! If you agree with my last modifications, then we can merge :) Note, I edited the test on ksathorfrac as nowadays read/write netdcf files can be unstable and cause test to hang. The values are different but this is because as we do not write, the catchment mask is not yet applied. |
|
@hboisgon, Ill go over it one last time, but it looks good! |
Issue addressed
Fixes number of smaller I/O issues
Fixes #276
Explanation
Bit and pieces of code.
Checklist
mainAdditional Notes (optional)
No.