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

DetectorChecksum.cpp: fix a typo #1172

Closed
wants to merge 2 commits into from
Closed

Conversation

veprbl
Copy link
Contributor

@veprbl veprbl commented Sep 20, 2023

BEGINRELEASENOTES

  • A typo was fixed in the string representation used for checksum calculation. Checksums reported by DD4hepDetectorChecksum plugin will change.

ENDRELEASENOTES

@andresailer andresailer enabled auto-merge (rebase) September 20, 2023 19:17
@github-actions
Copy link

github-actions bot commented Sep 20, 2023

Test Results

       6 files         6 suites   6h 11m 19s ⏱️
   356 tests    350 ✔️ 0 💤   6
1 058 runs  1 044 ✔️ 0 💤 14

For more details on these failures, see this check.

Results for commit 262c20c.

♻️ This comment has been updated with latest results.

@andresailer
Copy link
Member

So in the vein of "there are no trivial changes" this changes the checksum calculation.

@veprbl
Copy link
Contributor Author

veprbl commented Sep 22, 2023

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.

@andresailer
Copy link
Member

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.

@veprbl
Copy link
Contributor Author

veprbl commented Sep 22, 2023

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.

@MarkusFrankATcernch
Copy link
Contributor

@wdconinc
Hi Wouter,
AFAIK you are the only users of the detector checksum up to now. Could you comment if we can merge this one or not.
Clearly it would invalidate all existing checksums, but there are also reasons of beauty ;-)

@wdconinc
Copy link
Contributor

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... ;-)

@MarkusFrankATcernch
Copy link
Contributor

@wdconinc Thanks a lot Wouter. I will then fix the tests and update the repository.

@MarkusFrankATcernch
Copy link
Contributor

Changes incorporated in PR: #1201

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.

4 participants