ImperialCollegeLondon / virtual_ecosystem

This repository is the home for the codebase for the Virtual Ecosystem project.
https://virtual-ecosystem.readthedocs.io
BSD 3-Clause "New" or "Revised" License
9 stars 1 forks source link

493 hydrology add canopy evaporation #528

Open vgro opened 1 month ago

vgro commented 1 month ago

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

Key checklist

Further checks

vgro commented 1 month ago

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 commented 1 month ago

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

davidorme commented 1 month ago

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

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vgro commented 1 month ago

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)