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

Feature/bypass flow #321

Merged
merged 7 commits into from
Oct 2, 2023
Merged

Feature/bypass flow #321

merged 7 commits into from
Oct 2, 2023

Conversation

vgro
Copy link
Collaborator

@vgro vgro commented Sep 29, 2023

This PR adds preferential bypass flow, i.e. the flow that bypasses the soil matrix and drains directly to the groundwater. This represents the famous 'pipes'. The shape parameter will need some further investigation and sensitivity analysis.

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2023

Codecov Report

Merging #321 (d7d389a) into develop (1bd1a0d) will increase coverage by 0.00%.
Report is 9 commits behind head on develop.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #321   +/-   ##
========================================
  Coverage    96.52%   96.53%           
========================================
  Files           48       48           
  Lines         2388     2392    +4     
========================================
+ Hits          2305     2309    +4     
  Misses          83       83           
Files Coverage Δ
...irtual_rainforest/models/hydrology/above_ground.py 100.00% <100.00%> (ø)
...irtual_rainforest/models/hydrology/below_ground.py 100.00% <ø> (ø)
virtual_rainforest/models/hydrology/constants.py 100.00% <100.00%> (ø)
...ual_rainforest/models/hydrology/hydrology_model.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@davidorme davidorme self-requested a review October 2, 2023 09:23
Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - this makes a lot of sense and that LISFLOOD solution looks very pragmatic. It would be good to add citations and maybe a slightly fuller explanatation of the shape parameter. I didn't really get it until I went to look at
https://ec-jrc.github.io/lisflood-model/2_09_stdLISFLOOD_preferential-bypass

This does need docs to explain it as well - the graph at the bottom of that link is really useful. Is there a separate hydrology docs PR planned?

Copy link
Collaborator

@jacobcook1995 jacobcook1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vgro
Copy link
Collaborator Author

vgro commented Oct 2, 2023

LGTM - this makes a lot of sense and that LISFLOOD solution looks very pragmatic. It would be good to add citations and maybe a slightly fuller explanatation of the shape parameter. I didn't really get it until I went to look at https://ec-jrc.github.io/lisflood-model/2_09_stdLISFLOOD_preferential-bypass

This does need docs to explain it as well - the graph at the bottom of that link is really useful. Is there a separate hydrology docs PR planned?

There is no separate documentation for the hydrology model, I was hoping that the docstrings will cover the most important point and refer to more detailed sources, like LISFLOOD. I'm happy to expand further here (most functions probably need a bit more detailed descriptions as they get established).

@davidorme
Copy link
Collaborator

I think we'll need a user-facing doc page for the hydrology model at some point. That should probably explain the features and parameterisation, but that may be for a later date.

@vgro
Copy link
Collaborator Author

vgro commented Oct 2, 2023

I think we'll need a user-facing doc page for the hydrology model at some point. That should probably explain the features and parameterisation, but that may be for a later date.

I agree. I will put this on my list and do in a separate PR once all the main features are implemented.

@vgro vgro merged commit da5ceca into develop Oct 2, 2023
22 checks passed
@vgro vgro deleted the feature/bypass_flow branch October 2, 2023 13:50
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.

4 participants