Add parameterset catalogue #118
Conversation
|
Nice progress! @sverhoeven |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@Peter9192 I think the PR is ready for review. What do you think? |
They should be part fix for #124
| return parametersets | ||
|
|
||
|
|
||
| def available_parameter_sets(target_model: str = None) -> Iterable[str]: |
There was a problem hiding this comment.
ewatercycle.parameter_sets.available_parameter_sets(target_model="lisflood") currently returns a generator, which is not ideal for the public API (and not consistent with the example notebook). It would be nice if we can incorporate this example in one of our notebooks, and ideally it should return a list of parameterset names or a dict with them names as keys.
There was a problem hiding this comment.
Changed it to tuple of strings as that more closely matches to api notebook.
Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
|
@Peter9192 could you have another look? |
There was a problem hiding this comment.
Almost there. I made a suggestion for that warning message. Also two minor points about the notebooks:
download_example_parameter_setsfails when one of the parameterset dirs already exists. Might be nice to have a 'skip_existing' and 'force_overwrite' option. Now, the CFG is also not updated, and my CFG.reload results in an old config file.- can we change
idtodoiin the example API notebook as well, and maybe execute all the commands in the cell that's about the parameter sets, instead of having fake output in comments?
ewatercycle/models/lisflood.py
Outdated
| parameters = [ | ||
| ('IrrigationEfficiency', self._get_textvar_value('IrrigationEfficiency')), | ||
| ('IrrigationEfficiency', self._get_textvar_value('IrrigationEfficiency'), 0.75), |
There was a problem hiding this comment.
So here we pretend there's a default, even if it's not set in the model config? And then the model itself will fail?
There was a problem hiding this comment.
Yes, it will fail when area.map or area.nc file does not exist
There was a problem hiding this comment.
Reverted having a default in the model, will raise KeyError when MaskMap or IrrigationEfficiency can not be found in config.
An bmi model will return error when file from config does not exist
Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
This reverts commit f776fce.
Also changed MaskMap to str type
|
Kudos, SonarCloud Quality Gate passed! |
Here's an initial idea on how we could support a parameterset catalogue.
Adds
ewatercycle.parameter_sets.available_parameter_sets()+ewatercycle.parameter_sets.get_parameter_set()as shown in api notebookewatercycle.parameter_sets.download_example_parameter_sets()to download example parameter sets.Changes:
Lisflood.setup()see maskmap from lisflood parameterset to setup #121~/.config/.ewatercycle/ewatercycle.yamlto~/.config/ewatercycle/ewatercycle.yamlRemoved:
Not part of this PR: