Adding Snow Hysteresis and Icemelt model #18
Conversation
Adding Snow Hysteresis and Icemelt models. Also including a tutorial file for the new models.
There was a problem hiding this comment.
Hi, sorry for the delay of my review. The code generally looks good, not much to complain here. Again, as said before, I did not check the logic of the model routines, but I assume that you have done that.
What is missing is the integration of your additions into the docs. Currently, neither the new models nor the examples would be visible from the documentation. Here is what needs to happen:
- In docs/source/api/models.rst add sections for the new models you add, similar to the existing models.
- In docs/source/examples/ add an rst version of your notebook. I just figured out that I did not use autoconvert here (as we do in NeuralHydrology), so this requires a manual command from your side. In the terminal run
ipython nbconvert --to rst your.ipynb, this creates an rst version of the notebook that you can put into the examples directory of the docs. Once that is done, edit docs/source/examples/examples.rst to add a link to the new rst file (see the existing once for how to do this).
You can create the docs locally by creating the rtd_environment.yml, activate that Conda environment than go into the docs/ directory an run make html. This will create an html directory under docs/build. Just open the index.html in there and you see the current version of the docs. That way, you can easily see if your changes worked.
If you have any questions, feel free to shoot me an email.
|
Oh and one more thing: So far, I have a test for each model implementation that checks that the model, for a given set of inputs and model parameters, returns the expected output. The parameters, inputs and outputs where taken from other publications, usually published data / code from the original model developers themselves that I took. Since you are adding new models, do you mind creating tests as well with a defined set of parameters and inputs and have the outputs saved in the tests/data directory? |
|
Also note: I just merged a small PR that updates the environment files to a more recent Python version. Would be cool if you can also update your local environments to see if you see any additional deprecation warnings or other issues. |
Adding and modifying the documentation files for the Hysteresis and Ice models.
Co-authored-by: Frederik Kratzert <[email protected]>
Co-authored-by: Frederik Kratzert <[email protected]>
including validation files
There was a problem hiding this comment.
Is the example supposed to be included in this PR? Looking at the notebook itself, it seems to be specific to some other package (RRMPG - Alpine)? It also imports a few modules that are not part of the Python environment file from RRMPG (spotpy, pyet), which also relates to a few minor comments below. I don't mind keeping this notebook under examples but probably there is a better name than "Tutorial file for RRMPG - Alpine", which is the name currently displayed in the docs. Other option is to remove it.
Lastly: Even though it does not matter too much, but for a test, it is not really required to run a multi-year simulation. You only want to make sure that the model logic is implemented correctly, which should be possible to check from e.g. half a year (to include snow melt). But no worries keeping this.
rrmpg/models/cemaneigehystgr4j.py
Outdated
| import spotpy | ||
| from spotpy.objectivefunctions import kge |
There was a problem hiding this comment.
I tried running your tests locally and this library is not part of the current conda environment for RRMPG, hence the tests failed. Is there any reason why you prefer spotpy over the kge implementation in utils/metrics.py? I would prefer to keep a minimal amount of dependencies, since every dependency can potentially break the library. At least the tests pass if I simply replace the imported kge function from spotpy with the calc_kge function of utils/calc_kge.py
rrmpg/models/cemaneigehystgr4jice.py
Outdated
| from spotpy.objectivefunctions import kge | ||
| from scipy import optimize |
rrmpg/models/cemaneigehystgr4j.py
Outdated
| def calc_mse1(evaluation, simulation): | ||
| df = pd.DataFrame({"obs": evaluation, "sim": simulation}).dropna() | ||
| return calc_mse(df["obs"].values, df["sim"].values) | ||
|
|
||
| def kge1(evaluation, simulation): | ||
| df = pd.DataFrame({"obs": evaluation, "sim": simulation}).dropna() | ||
| return kge(df["obs"].values, df["sim"].values) |
There was a problem hiding this comment.
What is the reason for these two functions? You convert numpy arrays to a pandas dataframe only then to extract the data as a numpy array with .values. Why not directly call calc_mse (and calc_kge) in the if-elif-else statement above?
rrmpg/models/cemaneigehystgr4jice.py
Outdated
| def calc_mse1(evaluation, simulation): | ||
| df = pd.DataFrame({"obs": evaluation, "sim": simulation}).dropna() | ||
| return calc_mse(df["obs"].values, df["sim"].values) | ||
|
|
||
| def kge1(evaluation, simulation): | ||
| df = pd.DataFrame({"obs": evaluation, "sim": simulation}).dropna() | ||
| return kge(df["obs"].values, df["sim"].values) |
Thanks for the feedback! Changes: - removed unnecessary dependencies (now using utils/calc_kge); - reduced tutorial notebook; - updated the docs accordingly; Cheers
There was a problem hiding this comment.
Thanks, looks all fine and I checked offline that all tests passed (seems like I missed to implement CI tests in github). Also the docs compile and look good.
So I will go forward and merge this PR. Thanks again 100x times for improving this library with new models.
Following models are added:
Additional a tutorial notebook with an input csv is added.