[Minuit2] Fix external index usage in analytical Hessian#20936
[Minuit2] Fix external index usage in analytical Hessian#20936guitargeek merged 1 commit intoroot-project:masterfrom
Conversation
Test Results 22 files 22 suites 3d 10h 50m 44s ⏱️ Results for commit 8f29655. |
Fix external index usage in analytical Hessian and also add a unit test to cover this former bug. This problem exists since the introduction of external Hessians to Minuit2 in 888a767 in the ROOT 6.28 development cycle. This bug was discovered while investigating root-project#20913.
guitargeek
force-pushed
the
hessian_external_indexing_bug
branch
from
8f29655 to
4fb2d77
Compare
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
I would suggest disentangling the fix and the test into two commits. It will improve a bit the review experience.
|
Ah, okay I get now what you mean! In another PR you made a similar comment, and there I thought you meant "disentangle fix+test from unrelated changes". Sorry for the misunderstanding. Just to be clear for the future: you prefer test commit first or fix commit first? fix first, so that tests always pass, right? |
Yes, precisely! And if the message says "Add test for xxx", you already know what to expect in the review. By splitting, you could even revert the fix and replace with a new one, and you would expect the test to stay green. |
|
Damn, sorry it was not split up in two commits in the end! I have done this in my local branch, and realized too late that the PR branch was not synced 🙁 Sorry, I really didn't mean to ignore your request! I juggled too many open PRs and lost the overview.... |
|
Will this fix be backported to 6.36? |
No problem. 🙂 |
3 participants
Fix external index usage in analytical Hessian and also add a unit test to cover this former bug.
This problem exists since the introduction of external Hessians to Minuit2 in 888a767 in the ROOT 6.28 development cycle.
This bug was discovered while investigating #20913.
Closes #20954.