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

fix: mv id() to getObjectID().index because id() returns ObjectID, not uint_t, in podio v0.17.1 #1106

Merged
merged 4 commits into from
Nov 11, 2023

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

This adds forward compatibility with podio v0.17.1. See AIDASoft/podio#493

What kind of change does this PR introduce?

  • Bug fix (issue: compilation falures with podio 0.17.1)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No. The modified algorithms are not relying on the most significant bits in the previous ObjectID uint_t that encode the collectionID.

@wdconinc wdconinc requested a review from veprbl November 11, 2023 18:55
@github-actions github-actions bot added topic: calorimetry relates to calorimetry topic: PID Relates to PID reconstruction labels Nov 11, 2023
@wdconinc wdconinc changed the title fix: mv id() to getObjectID().index because id() now returns ObjectID, not uint_t fix: mv id() to getObjectID().index because id() returns ObjectID, not uint_t, in podio v0.17.1 Nov 11, 2023
@wdconinc wdconinc enabled auto-merge November 11, 2023 18:56
@wdconinc wdconinc force-pushed the podio-objectID-id-to-index branch from 462e86b to ef9e08b Compare November 11, 2023 20:26
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

For correctness sake, we should be leaning towards full object id comparisons where possible. This looks correct, still.

@wdconinc wdconinc added this pull request to the merge queue Nov 11, 2023
@wdconinc
Copy link
Contributor Author

For correctness sake, we should be leaning towards full object id comparisons where possible.

Fortunately that is already the case in several other places. I didn't (need to) replace those cases because the ObjectID comparison works fine. It's only the places where the ObjectID is explicitly assumed to be a uint_t (for storing or debug output) that are modified here.

Merged via the queue into main with commit 01e1d5a Nov 11, 2023
74 checks passed
@wdconinc wdconinc deleted the podio-objectID-id-to-index branch November 11, 2023 23:57
@veprbl veprbl added the backport v1.7 Backport into v1.7 label Nov 14, 2023
@epic-capybara
Copy link

veprbl pushed a commit that referenced this pull request Nov 15, 2023
…rns ObjectID, not uint_t, in podio v0.17.1 (#1109)

# Description
Backport of #1106 to `v1.7`.

---------

Co-authored-by: Wouter Deconinck <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport v1.7 Backport into v1.7 topic: calorimetry relates to calorimetry topic: PID Relates to PID reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants