959 sort out time interval in energy balance#988
Conversation
…d negative values
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #988 +/- ##
===========================================
+ Coverage 95.11% 95.13% +0.02%
===========================================
Files 80 80
Lines 6693 6724 +31
===========================================
+ Hits 6366 6397 +31
Misses 327 327 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…9-sort-out-time-interval-in-energy-balance' of https://github.com/ImperialCollegeLondon/virtual_rainforest into 959-sort-out-time-interval-in-energy-balance
jacobcook1995
left a comment
jacobcook1995
left a comment
There was a problem hiding this comment.
Had a few queries, but the general structure looks sensible to me
|
|
||
| # If a valid upper value and a mixing coefficient are found, compute flux | ||
| if upper_i >= 0 and np.isfinite(mixing_coefficient[upper_i, j]): | ||
| delta_up = input_variable[upper_i, j] - center_val |
There was a problem hiding this comment.
Should one of delta_up or delta_down be reversed? Assuming a positive or negative gradient through the canopy, one of the delta values is going to yield negative values and then gets clipped by flux_up < 0
There was a problem hiding this comment.
I removed the clipping step to allow for flows in both directions
| flux_down = 0.0 | ||
|
|
||
| # ---- Update value ---- | ||
| change = (flux_up + flux_down) * time_interval / layer_thickness[i, j] |
There was a problem hiding this comment.
Is this a double correction for layer thickness? The fluxes have already been divided through by thickness.
…//github.com/ImperialCollegeLondon/virtual_rainforest into 959-sort-out-time-interval-in-energy-balance
|
I have made the suggested changes and added an extra step to ensure that the values stay within realistic limits (especially important for atmospheric humidity). I tried to vectorize where possible but couldn't find a way around the last for loop. |
davidorme
left a comment
davidorme
left a comment
There was a problem hiding this comment.
This looks good. I did have some difficulty getting my head around the new bit to redistribute values outside limits. I think that's partly because that process is embedded within mix_and_ventilate so it's hard to visualise/troubleshoot.
I've put in a PR (#1011) that breaks this part out into its own standalone clamping function and tests it separately - I can't get rid of the loop, but I think it can be calculated more simply. This feels like a better unit structure for the functionality. If that PR looks good to you, then I think this is ready to go?
Looks good to me, I just merged it. Could you please approve this PR when you're happy with it? |
4 participants
This PR replaced the placeholder '1's with the actual time interval in the microclimate update.
To make this work with the varying number of canopy layers, I also fixed how the above ground layer thickness is calculated, a few directions of fluxes, and adjusted the vertical mixing - basically all the steps where a value from below is called and NaN has to be skipped.
I also tidied the tests a bit to check if the values fall in a sensible range instead of specific values and introduced a step to mask the nan values in the canopy. The next PR will be testing with a grid cell that has no vegetation at all.
Fixes #959
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks