-
Notifications
You must be signed in to change notification settings - Fork 100
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
DetectorChecksum.cpp: fix a typo #1172
Conversation
So in the vein of "there are no trivial changes" this changes the checksum calculation. |
Fair point. My understanding was that the feature was experimental. Also, it's not clear that avoiding false negatives is part of the contract here, users should expect checksums mismatch for no apparent reason. In any case, I've added changelog notice, then a decision can be made when and if to merge this. |
Maybe we should have a runtime flag (Environment variable) that makes this switch, and then people can check if the checksum is actually due to our change here, or something else? And in the next version, we make this the default. |
I would assume that users need to be switched to the latest checksumming version ASAP. Supporting calculating with an older algorithm variants is a fine idea. I can look into implementing this. |
@wdconinc |
Our reasons for the checksum are consistency usually within the same version of dd4hep, i.e. making sure that the same geometry was used on simulation and reconstruction, but usually within the same environment and with the same version of dd4hep. I think we would have no problem with changing the checksum with this PR. I hope someone ran a spell-check so we don't find another typo the day after merging this... ;-) |
@wdconinc Thanks a lot Wouter. I will then fix the tests and update the repository. |
Changes incorporated in PR: #1201 |
BEGINRELEASENOTES
ENDRELEASENOTES