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

extraction fix #758

Merged
merged 1 commit into from
Sep 12, 2024
Merged

extraction fix #758

merged 1 commit into from
Sep 12, 2024

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Sep 6, 2024

Purpose

Update root extraction so that it tends to zero as the soil dries or freezes.
We have the Darcy flux:
Flux = -K_eff [(psi_canopy - psi_soil)/(z_canopy -z_soil) + 1]

Previously: K_eff = [K_canopy(psi_soil) + K_canopy(psi_canopy)]/2.

Two things have changed: we now use the harmonic mean, and we replace K_canopy(psi_soil) with K_soil(psi_soil). Note that K_soil() and K_canopy() are two different functions.

Currently: K_eff = K_soil(psi_soil) * K_canopy(psi_canopy)/ [K_soil(psi_soil) + K_canopy(psi_canopy)]

So, if K_soil(psi_soil) -> 0, then K_eff -> 0, and the root extraction goes to zero consistently. Although as psi_soil ->0, K_canopy(psi_soil) ->0, it may not drop to zero quickly enough due to the differing functional forms.

Please see CLM:
Screenshot 2024-09-10 at 15 24 54
which is different in that it uses conductances and has canopy -> root, but see Equation 11.16 (harmonic mean) and Equation 11.18 (K_soil in soil layer, i.e. K_soil(psi_soil))

Effect on NaNs:

Global run after soil evaporation fix (very similar to current main):
swc_2 88576e7_new
Global run after these changes:
image

To-do

Review

Content

  1. Change the root extraction to depend on K_soil, and not K_plant(psi_soil). Note that this is only changed in the prognostic soil +canopy mode, and not the standalone canopy mode (prescribed soil). In the latter, we use K_plant(psi_soil) still.
  2. Use the geometric mean to compute K_eff, and not arithmetic mean
  3. Update tests

  • I have read and checked the items on the review checklist.

@kmdeck kmdeck force-pushed the kd/root_extraction_nans branch from 7cbc3fb to 9158f92 Compare September 10, 2024 22:55
@kmdeck kmdeck requested a review from braghiere September 10, 2024 22:58
@kmdeck kmdeck self-assigned this Sep 10, 2024
@kmdeck kmdeck requested a review from juliasloan25 September 10, 2024 23:20
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

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

Awesome!! This looks great, I just had a couple comments :)

.buildkite/longruns_gpu/pipeline.yml Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
test/standalone/Vegetation/plant_hydraulics_test.jl Outdated Show resolved Hide resolved
@kmdeck
Copy link
Member Author

kmdeck commented Sep 11, 2024

Change the root extraction to depend on K_soil, and not K_plant(psi_soil). Note that this is only changed in the prognostic soil +canopy mode, and not the standalone canopy mode (prescribed soil). In the latter, we use K_plant(psi_soil) still.

It seems unusual to me to use slightly different equations for standalone vs integrated modes. Although maybe it doesn't make sense to evaluate K_soil with prescribed soil?

Yeah, I agree...it would entail changing the PrescribedSoil struct to include a function for K_soil(psi) as well, or a timeseries of Ksoil(t) which is consistent with psi_soil(t), which would be made up at this point. We dont really have a use case for it yet.

@braghiere
Copy link
Member

Change the root extraction to depend on K_soil, and not K_plant(psi_soil). Note that this is only changed in the prognostic soil +canopy mode, and not the standalone canopy mode (prescribed soil). In the latter, we use K_plant(psi_soil) still.

It seems unusual to me to use slightly different equations for standalone vs integrated modes. Although maybe it doesn't make sense to evaluate K_soil with prescribed soil?

Yeah, I agree...it would entail changing the PrescribedSoil struct to include a function for K_soil(psi) as well, or a timeseries of Ksoil(t) which is consistent with psi_soil(t), which would be made up at this point. We dont really have a use case for it yet.

Agree. If it is not too difficult, maybe we can change the standalone as well? If it is a bit of work, we can leave it for now since we're focusing on canopy+soil development, which is more aligned to OKRs.

Copy link
Member

@braghiere braghiere left a comment

Choose a reason for hiding this comment

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

It looks good to me. Left a few comments.

experiments/long_runs/land.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
@juliasloan25
Copy link
Member

Change the root extraction to depend on K_soil, and not K_plant(psi_soil). Note that this is only changed in the prognostic soil +canopy mode, and not the standalone canopy mode (prescribed soil). In the latter, we use K_plant(psi_soil) still.

It seems unusual to me to use slightly different equations for standalone vs integrated modes. Although maybe it doesn't make sense to evaluate K_soil with prescribed soil?

Yeah, I agree...it would entail changing the PrescribedSoil struct to include a function for K_soil(psi) as well, or a timeseries of Ksoil(t) which is consistent with psi_soil(t), which would be made up at this point. We dont really have a use case for it yet.

Agree. If it is not too difficult, maybe we can change the standalone as well? If it is a bit of work, we can leave it for now since we're focusing on canopy+soil development, which is more aligned to OKRs.

I think it makes sense to keep PrescribedSoil as-is for now, until we do have a use case that necessitates this change

@kmdeck
Copy link
Member Author

kmdeck commented Sep 12, 2024

Change the root extraction to depend on K_soil, and not K_plant(psi_soil). Note that this is only changed in the prognostic soil +canopy mode, and not the standalone canopy mode (prescribed soil). In the latter, we use K_plant(psi_soil) still.

It seems unusual to me to use slightly different equations for standalone vs integrated modes. Although maybe it doesn't make sense to evaluate K_soil with prescribed soil?

Yeah, I agree...it would entail changing the PrescribedSoil struct to include a function for K_soil(psi) as well, or a timeseries of Ksoil(t) which is consistent with psi_soil(t), which would be made up at this point. We dont really have a use case for it yet.

Agree. If it is not too difficult, maybe we can change the standalone as well? If it is a bit of work, we can leave it for now since we're focusing on canopy+soil development, which is more aligned to OKRs.

I think it makes sense to keep PrescribedSoil as-is for now, until we do have a use case that necessitates this change

Thanks guys! I updated an old issue to have this included: #159

@kmdeck
Copy link
Member Author

kmdeck commented Sep 12, 2024

@braghiere @juliasloan25 I will extend the run to 365 days (or document why we cant and open an issue, I forget why it didnt run before).

would this be ready to merge then? If so, if you approve it now Ill merge it after the last fix to set tf=365 days.

I would really like to rethink aspects of our plant hydraulics model thoroughly #159 , but for now want to move forward with what we have

@juliasloan25
Copy link
Member

@braghiere @juliasloan25 I will extend the run to 365 days (or document why we cant and open an issue, I forget why it didnt run before).

would this be ready to merge then? If so, if you approve it now Ill merge it after the last fix to set tf=365 days.

I would really like to rethink aspects of our plant hydraulics model thoroughly #159 , but for now want to move forward with what we have

Yeah, looks good to me!

@kmdeck kmdeck force-pushed the kd/root_extraction_nans branch from 4ed399d to 8208c89 Compare September 12, 2024 17:46
Copy link
Member

@braghiere braghiere left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@kmdeck kmdeck merged commit 8f582be into main Sep 12, 2024
11 of 12 checks passed
@kmdeck kmdeck deleted the kd/root_extraction_nans branch September 12, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants