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

Improve derived variable init for const pointers #919

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

vaithak
Copy link
Collaborator

@vaithak vaithak commented Jun 4, 2024

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

Copy link
Contributor

github-actions bot commented Jun 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

@vaithak
Copy link
Collaborator Author

vaithak commented Jun 4, 2024

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
Copy link
Collaborator Author

vaithak commented Jun 4, 2024

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

@vaithak
Copy link
Collaborator Author

vaithak commented Jun 4, 2024

The issue was that while calling pullbacks, we don't maintain non-differentiable arguments. @PetroZarytskyi had fixed this for most of the cases here: #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.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #919   +/-   ##
=======================================
  Coverage   94.07%   94.08%           
=======================================
  Files          53       53           
  Lines        7747     7757   +10     
=======================================
+ Hits         7288     7298   +10     
  Misses        459      459           
Files Coverage Δ
lib/Differentiator/ReverseModeVisitor.cpp 97.16% <100.00%> (+0.01%) ⬆️
Files Coverage Δ
lib/Differentiator/ReverseModeVisitor.cpp 97.16% <100.00%> (+0.01%) ⬆️

Copy link
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM but I'd wait for @PetroZarytskyi's review.

@PetroZarytskyi
Copy link
Collaborator

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

@vgvassilev vgvassilev merged commit b38d8cf into vgvassilev:master Jun 6, 2024
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants