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

493 hydrology add canopy evaporation #528

Draft
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

vgro
Copy link
Collaborator

@vgro vgro commented Jul 24, 2024

This turns out to be more complex than anticipated. In order to calculate canopy evaporation, we need a range of variables that are provided by the abiotic model. These are currently not in the abiotic_simple model; the attempt to add them caused a crash in ve_run. This is a Draft PR to see error message.

I have tried to reproduce this step by step and it all works fine until the this commit, unit tests are fine but the integration tests runs into nowhere when it tried to update the self.data['wind_speed'] in line 205 in abiotic_simple_model.py. (After that something went funny with pushing and pulling and I had weird duplications of lines, I hope this is all sorted)

@dalonsoa and @davidorme could you take a look into this, maybe you spot something? I suspect it has to do with the order of models, it work ok when the wind comes from the hydrology model, but it should work with the changes in variables?

Fixes # (issue)

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

@vgro vgro linked an issue Jul 24, 2024 that may be closed by this pull request
@vgro
Copy link
Collaborator Author

vgro commented Aug 5, 2024

An update on this, I have tried to run the ve_run example locally, it also get's stuck but interestingly when it tries to update the soilmodel, see logfile attached. @davidorme do you have an idea how I could get to the bottom of this?
logfile.log

@vgro vgro requested review from davidorme and dalonsoa August 5, 2024 15:37
@vgro
Copy link
Collaborator Author

vgro commented Aug 6, 2024

We have identified the issue, see #536.
Will work on this and then put it up for review.

@davidorme
Copy link
Collaborator

I'll de-request those outstanding reviews for now.

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.02%. Comparing base (0cf635a) to head (ea27c75).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #528      +/-   ##
===========================================
+ Coverage    94.96%   95.02%   +0.05%     
===========================================
  Files           73       73              
  Lines         4071     4098      +27     
===========================================
+ Hits          3866     3894      +28     
+ Misses         205      204       -1     

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

@vgro
Copy link
Collaborator Author

vgro commented Aug 9, 2024

Quick update (mostly for me to pick up after summer break):

The problem with the endless loop in ve_run is solved; I added a tentative check in the soil model, will need to be discussed for soil, see #536 with @jacobcook1995, and more generally for integration tests

The problem was in the wind update, there seemed to be an issue with filling the right layers with zeros for the sensible heat flux variable. It was fixed with a hack in the abiotic_simple model to continue working, but came back with the abiotic model - the first canopy layer was Nan for all temperatures and fluxes. This points to the general issue with the inconsistencies in the abiotic model subcomponents. Hopefully, the conference will shed light in this.

Fixes to do (potentially in separate PR):
[ ] integrate model versions better (constants, wrapper functions, same order, same varaibles tec)
[ ] fix layer selection for sensible heat flux for both model versions
[ ] add wind to abiotic init and check why it causes first canopy layer to be nan
[ ] update tests for both models to capture the additional variables that are returned
[ ] add checks for Nan in key places to stop process early

To do's for evaporation:
[x] add aero resistance canopy
[ ] add potential evaporation (drafted)
[ ] add canopy evaporation function (drafted)

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.

Hydrology - Add canopy evaporation
3 participants