-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hydrology model #6
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @rcw5890 -- thanks very much!
A few thoughts inline. Comments on the notebook:
- can we include the ODEs in the notebook itself?
- can we include a reference to the paper that introduced the ODE in the first place?
- can we show a plot in the notebook that shows the time series vs the modelled values?
- is it perhaps better to have an examples folder in the main repo directory where we provide examples for all models in the database?
README.md
Outdated
|
||
While the `pints.toy` module already includes a number of simple models and distributions, this repository is intended for more complex models associated with real experimental data. | ||
|
||
## Models and data | ||
|
||
Hydrology | ||
[Rainfall runoff model and river discharge data for the French Broad River at Asheville, North Carolina.](streamflow/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include citations for the model? Also, can we create a sort of table that says why each of the models is interesting? So, here, I guess it's because of potential model misspecification / a potentially complex noise process / numerical error in solving the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea, I started making a table which might work
|
||
Evap = measured or theoretical evaporation (provided as input to the model) | ||
|
||
InterceptEvap = evaporation from the interception component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does interception component mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some extra explanations for these terms (also for the two comments below)
|
||
.. math:: | ||
f(S, a) = \frac{1 - e^{-aS}}{1 - e^{-a}} | ||
* S_i = interception storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is interception storage?
.. math:: | ||
f(S, a) = \frac{1 - e^{-aS}}{1 - e^{-a}} | ||
* S_i = interception storage | ||
* S_u = unsaturated storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does unsaturated storage mean?
streamflow/pystreamflow/model.py
Outdated
|
||
* FastStream(t) = S_f / K_f | ||
|
||
Models of this type are described in [1]_ and [2]_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these models of this type or are they this specific model? In other words, did we extend / change their model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model is the same as [1], but that paper doesn't actually have many details, which is why I mentioned [2], which has helpful details for a slightly different model. I've tried to explain this better now.
|
||
1. Navigate to `differential-equations-inference-db/streamflow/` (this directory) | ||
1. For a simple install, run `pip install .` | ||
1. To install with support for the CVODE solver via scikits.odes, run `pip install .[cvode]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I tried to install without doing this step but I ran into the following error:
RuntimeError: scikit solver could not be imported
Is scikit.odes required? Or does it just make things faster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, when exactly did this error appear? Was it when running the notebook? Or did it appear during pip install?
scikit.odes is optional and just makes solving faster. However, if you try to instantiate the model object with the non-default optional argument solver='scikit'
, it will raise this error if scikits.odes is not available. So if this happened when running the notebook cell which compares the two different solvers, I would consider that the expected behavior.
Another option would be to raise a warning and revert to the scipy solver in this case.
Thanks for the comments @ben18785 , I'll start working through them |
These changes make sense to me, and should be in the latest commit, except for the plot showing time series vs modelled values---I have the figure from |
Add the hydrology model and French Broad River data
Closes #7