search

vgvassilev / clad

clad -- automatic differentiation for C/C++
GNU Lesser General Public License v3.0
266 stars 113 forks source link

Improve derived variable init for const pointers #919

Closed vaithak closed 4 weeks ago

vaithak commented 1 month ago

This will fix the segmentation fault in roofit benchmarks (when auxiliary arrays are removed).

github-actions[bot] commented 1 month ago

clang-tidy review says "All clean, LGTM! :+1:"

vaithak commented 1 month ago

cc: @guitargeek. I have tested this with the atlas likelihood benchmark code, it now works after removing the auxiliary arrays. The runtime results have also improved because of this.

vaithak commented 1 month ago

The failed tests are due to the CUDA download server not being available.

vaithak commented 1 month ago

The issue was that while calling pullbacks, we don't maintain non-differentiable arguments. @PetroZarytskyi had fixed this for most of the cases here: https://github.com/vgvassilev/clad/pull/802, but it still had some more complex cases which required activity analysis, as he mentioned in the PR too. My PR just tries to improve upon that by handling the case for const pointers, which can be handled without activity analysis too.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.08%. Comparing base (9579ce4) to head (3516cbc).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/vgvassilev/clad/pull/919/graphs/tree.svg?width=650&height=150&src=pr&token=9f6Q4em8hE&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev)](https://app.codecov.io/gh/vgvassilev/clad/pull/919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) ```diff @@ Coverage Diff @@ ## master #919 +/- ## ======================================= Coverage 94.07% 94.08% ======================================= Files 53 53 Lines 7747 7757 +10 ======================================= + Hits 7288 7298 +10 Misses 459 459 ``` | [Files](https://app.codecov.io/gh/vgvassilev/clad/pull/919?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | Coverage Δ | | |---|---|---| | [lib/Differentiator/ReverseModeVisitor.cpp](https://app.codecov.io/gh/vgvassilev/clad/pull/919?src=pr&el=tree&filepath=lib%2FDifferentiator%2FReverseModeVisitor.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev#diff-bGliL0RpZmZlcmVudGlhdG9yL1JldmVyc2VNb2RlVmlzaXRvci5jcHA=) | `97.16% <100.00%> (+0.01%)` | :arrow_up: | | [Files](https://app.codecov.io/gh/vgvassilev/clad/pull/919?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | Coverage Δ | | |---|---|---| | [lib/Differentiator/ReverseModeVisitor.cpp](https://app.codecov.io/gh/vgvassilev/clad/pull/919?src=pr&el=tree&filepath=lib%2FDifferentiator%2FReverseModeVisitor.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev#diff-bGliL0RpZmZlcmVudGlhdG9yL1JldmVyc2VNb2RlVmlzaXRvci5jcHA=) | `97.16% <100.00%> (+0.01%)` | :arrow_up: |
PetroZarytskyi commented 1 month ago

Looks good to me. Sure, this will probably become obsolete once we have activity analysis but this is a great simplification for now.