Make the Root dynamics differentiable#29
Conversation
fnattino
left a comment
fnattino
left a comment
There was a problem hiding this comment.
Nice @SarahAlidoost, looks great to me! I have just added a couple of minor comments, feel free to include only what makes sense to you and let me know whether anything is unclear!
|
|
||
| # Increase in root biomass | ||
| r.GRRT = mask * k.FR * k.DMI | ||
| r.DRRT = mask * s.WRT * p.RDRRTB(k.DVS) |
There was a problem hiding this comment.
I think that the values returned by the Afgen objects (here the call to p.RDRRTB) are not tensors. For the scalar case it probably makes no difference, but it will probably matter when we want to vectorize this (#19).
There was a problem hiding this comment.
I have seen that in leaf_dynamics we convert the output of the call to Afgen objects to tensor:
But a better approach might be to modify
pcse.util.Afgen so that it returns a tensor already. We could maybe use one of the functions for piece-wise linear interpolation from here. Anyway, not to be addressed here, it can probably be done as part of #19.
There was a problem hiding this comment.
@fnattino thanks! You are right. Some of the parameters are not yet fully tensors. This PR is about one parameter: TDWI. The rest should be fixed in the next iteration, not only in #19 but when we revisit this module for the differentiability of other parameters. Note that currently, only TDWI is covered by tests.
Co-authored-by: Francesco Nattino <49899980+fnattino@users.noreply.github.com>
Co-authored-by: Francesco Nattino <49899980+fnattino@users.noreply.github.com>
Co-authored-by: Francesco Nattino <49899980+fnattino@users.noreply.github.com>
github-project-automation
Bot
moved this from In review
to Done
in DeltaCrop: epic1 and epic2
3 participants
relates #8
🔴 this branch is based on #12 , should be merged after that.