Conversation
There was a problem hiding this comment.
@SCiarella thanks. The changes looks good. The PR needs a few modifications specially for the vecorization which its tests currently missing. Please see my comments, if something unclear, let me know.
SCiarella
left a comment
There was a problem hiding this comment.
👋 @SarahAlidoost Thanks for the careful review!
I have implemented all the suggestions, except for a few points that remain open:
- [1] I have cleaned the
test_root_dynamicsas you suggested, but I have kept the single-testclass structure because I think it is easier to extend and maintain and it is not hard to interpret, but let me know what you think - [2] I do not fully understand the vectorization requirement. To me it is already vectorized (parameter can be passed as tensors). Can you give me a practical example?
- [3] should we ask someone to confirm that the gradient
TWRT->RDRRTBis numerically 0?
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Fix duplicated mask
|
With the last commit I have implemented the vectorization for the root dynamics. Finally, I have copied all the vectorization tests that we use for the leaf dynamics and implemented them in |
SarahAlidoost
left a comment
There was a problem hiding this comment.
@SCiarella thanks for addressing the comments 👍 We need a few more tests to make sure everything works with vectorization:
- Tests for gradients when parameters are vectors, see my code suggestions in class
TestDiffRootDynamicsGradients. - Tests for parameter "RDRRTB", see the list in
test_root_dynamics_with_one_parameter_vector. - Tests for other parameters in
test_root_dynamics_with_multiple_parameter_vectors, andtest_root_dynamics_with_multiple_parameter_arrays. Currently, only two parameters ("RDI", "RRI") are there.
Please have a look and let me know if something is unclear. Thanks.
Extend tests
|
Thanks @SarahAlidoost! Now I have fully vectorized also Notice that I have also added a branch to |
SarahAlidoost
left a comment
There was a problem hiding this comment.
@SCiarella thanks for addressing the comments, nice work 💪 I have one suggestion on refactoring of numerical function making it more generic, see my suggestion on the code. If you agree with it, please go ahead with committing it and merge this PR 🚀
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
|



This PR addresses the differentiability requirement of issue #47.
All the parameters except
RDRRTBwere already differentiable since we have defined them astorch.tensor.To make
RDRRTBdifferentiable, I had to redefinepcse.utils.Afgento use tensors instead oflist. For this reason I have addedsrc/physical_models/crop/afgen.pywhich is now differentiable.I have also restructured
test_root_dynamicssuch that all the combinations of parameter-output are checked, and code duplication is reduced.As a quality of life improvement, I have installed
pre-committhat automatically runsruffchecks before eachgit commit.