Add simple SIF model to photosynthesis #706
Conversation
AlexisRenchon
force-pushed
the
kd/rb/ar/basic_SIF
branch
from
c0709f1 to
e86fd4d
Compare
| @@ -158,6 +158,7 @@ function photosynthesis_ozark(; | |||
| ΔHΓstar = FT(37830), | |||
| ΔHJmax = FT(43540), | |||
| ΔHRd = FT(46390), | |||
| sif_parameters = SIFParameters{FT}(), | |||
| ) | |||
| return FarquharParameters( | |||
There was a problem hiding this comment.
do we need to pass sif_parameters here? I thought in our methods in CreateParametersExt we have a default which makes the sif_params and uses them
There was a problem hiding this comment.
We need it here because it calls the struct, not the function.
Note that we plan to refactor ClimaLandSimulations (with the help of Gabriele), to have it better designed in general, with PFT framework, global simulations... and ready for ClimaCalibrate
| J = electron_transport(APAR, Jmax, θj, ϕ) | ||
| (; kf, kd_p1, kd_p2, min_kd, kn_p1, kn_p2, kp, kappa_p1, kappa_p2) = | ||
| sif_parameters | ||
| Tf = FT(273.15) |
There was a problem hiding this comment.
this is kind of annoying, but we should get T_freeze from the earth_param_set in Canopy.jl update_aux!, and pass as an argument... like we do with R
There was a problem hiding this comment.
Let's do that together!
I am not sure of how to do it correctly, do you mean have Tf as an argument of compute_SIF_at_a_point and get that argument inside Canopy.jl update_aux!?
Is this to reduce allocation? or make sure we use params instead of hard coding?
| update_photosynthesis!( | ||
| Rd, | ||
| An, | ||
| Vcmax25, | ||
| SIF, |
There was a problem hiding this comment.
Can we make these two things independent? Why does update_photosynthesis! needs SIF?
There was a problem hiding this comment.
we could, it just currently lives in the photosynthesis name space (stored in canopy.photosynthesis.SIF), we thought it was okay for now with a simple model...
Do you suggest making a new canopy output / model, stored in canopy.SIF, in a Vegetation/SIF.jl file? (similar to what we have, photosynthesis.jl, radiation.jl, stomatalconductance.jl, etc.)
If we do, it will be a bit more involved and a breaking change, but it will allow to have multiple SIF modules
There was a problem hiding this comment.
maybe we could do that for a next PR?
There was a problem hiding this comment.
Yes. I think it should be an independent module. If anything it is a radiative variable and it should live in radiation instead of photosynthesis.
There was a problem hiding this comment.
Like SIF would be equivalent to APAR. We measure it with a radiometer.
| J = electron_transport(APAR, Jmax, θj, ϕ) | ||
| (; kf, kd_p1, kd_p2, min_kd, kn_p1, kn_p2, kp, kappa_p1, kappa_p2) = | ||
| sif_parameters | ||
| Tf = FT(273.15) |
Purpose
Add a simple SIF model to photosynthesis
To do: