Skip to content
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

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open

Hydrology model #6

wants to merge 44 commits into from

Conversation

rccreswell
Copy link
Collaborator

@rccreswell rccreswell commented Mar 25, 2021

Add the hydrology model and French Broad River data
Closes #7

@rccreswell rccreswell marked this pull request as draft March 25, 2021 21:57
@rccreswell rccreswell marked this pull request as ready for review March 29, 2021 08:27
Copy link
Contributor

@ben18785 ben18785 left a 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 Show resolved Hide resolved
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/)
Copy link
Contributor

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.

Copy link
Collaborator Author

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
Copy link
Contributor

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?

Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Contributor

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 Show resolved Hide resolved

* FastStream(t) = S_f / K_f

Models of this type are described in [1]_ and [2]_.
Copy link
Contributor

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?

Copy link
Collaborator Author

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]`
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@rccreswell
Copy link
Collaborator Author

Thanks for the comments @ben18785 , I'll start working through them

@rccreswell
Copy link
Collaborator Author

  • 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?

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 pints.plot.series as the last entry in the notebook, is there something additional you were thinking for this? Alternatively, maybe this figure should be moved upwards in the notebook to be more visible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement rainfall runoff model
2 participants