823 dont allow rainfall to be negative

[vgro]

This PR fixes negative rainfall and adds error logging if it should happen again. I had calculated the evaporation and leaf drainage from each canopy layer with the condition that this should not exceeds the interception pool. In the next step, I summed over all layers and subtracted that from the rainfall, I believe this was the problem. Now I sum first to make sure the sum is smaller than the available water.

Fixes #823

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist

• Make sure you've run the pre-commit checks: $ pre-commit run -a
• All tests pass: $ poetry run pytest

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works
• Relevant documentation reviewed and updated

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.76%. Comparing base (2ec7f4a) to head (8028cd3).
⚠️ Report is 2136 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #843   +/-   ##
========================================
  Coverage    94.75%   94.76%           
========================================
  Files           75       75           
  Lines         5550     5559    +9     
========================================
+ Hits          5259     5268    +9     
  Misses         291      291           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jacobcook1995]

LGTM! The function to automatically catch this type of error if it occurs again feels like a really good idea for helping with calibration

[davidorme]

I think we should look again at implementing the bounds checking for this too - running the simulation in bounds checking mode would be really useful for calibration.

[vgro]

I think we should look again at implementing the bounds checking for this too - running the simulation in bounds checking mode would be really useful for calibration.

I agree, but future problem :-)

[sallymatson]

Question about this error message. Since (I believe) the precipitation is a calculated/derived value, how would a modeller reconcile this issue? Is it possible to point them to how to solve it (e.g. "Surface precipitation is negative. Consider checking XX input variable to reconcile.")

Also, should this be a fatal error? It seems here that this error will write to the log but not stop the simulation. But what will that mean going forward for the next runs?

Not requesting changes, just confirming that this is the best way to alert - I leave final judgement to you! And I guess if the simulation now prevents it, then maybe it's not necessary to change.

[davidorme]

We've got a long-standing plan to implement something like this as broader bounds checking. This discussion is old (#135) but I think we'd now add hard or soft bounds data to variables.toml. Those are going to be useful values for users anyway, and then if we run in bounds mode, all changes to a variable in data can be bounds checked. Hard bounds raise, soft bounds warn.

But - as @vgro says: a problem for another day.

[sallymatson]

Cool. Seems very valuable for calibration..... maybe something for me to work on once stochiometry is figured out.

[vgro]

Good point, I agree that the simulation should stop when this happens and some more info should be given to solve the problem.