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

RecoParticleVertexAssociation: rename relation members for consistency between association #134

Closed
wants to merge 3 commits into from

Conversation

fdplacido
Copy link
Contributor

BEGINRELEASENOTES

  • Rename last association to be consistent with all other associations.

ENDRELEASENOTES

This allows to use any association in a generic (templates) way, so all the methods will be called the same.

@fdplacido fdplacido requested a review from vvolkl December 15, 2021 14:51
Copy link
Collaborator

@vvolkl vvolkl left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@tmadlener
Copy link
Contributor

Makes sense to me as well. Should we consider having more generic names than rec and sim then? I think all but this relation describe an association to an MC particle, so there it makes sense, but with the vertex it could be a bit confusing.

Depending on how many places this occurs in, one could also consider to have some if constexpr in place to handle the RecoParticleVertexAssociation just slightly different than the rest.

In any case this is likely a temporary solution until there is a more generic notion of relations/associations in podio, right?

@fdplacido
Copy link
Contributor Author

Maybe we should rename all association elements to "from" and "to", which seems the most generic thing to put. Since those are part of a collection named "*Association" should be pretty clear what that refers to, no?

@tmadlener
Copy link
Contributor

I think renaming to "to" and "from" could be done as a preparatory step before there is proper support for this in podio. The only concern I would have here is that it then becomes slightly harder to use these associations in "both directions"; e.g. in ILD workflows there is usually an association from MC to Reco and another one from Reco to MC with different weights. This is then used for example to calculate reconstruction efficiencies with one of those and fake rates with the other. But I don't think this would be a major showstopper at the moment.

@vvolkl vvolkl changed the title rename for consistency between association RecoParticleVertexAssociation: rename relation members for consistency between association Mar 9, 2022
@fdplacido fdplacido force-pushed the rename_association branch 2 times, most recently from 12a0a2b to 35705f8 Compare March 14, 2022 15:48
@tmadlener
Copy link
Contributor

This would not break I/O, but probably some code that still has rec and sim as relation names. I have no strong preference on whether this should go in before a new tag. We will have to fix the naming of the code that uses this in any case.

@tmadlener
Copy link
Contributor

Can you quickly rebase, to get rid of the "out-of-date with the base branch"?

@fdplacido fdplacido force-pushed the rename_association branch from 62cd348 to c8dc545 Compare March 18, 2022 13:42
Comment on lines +428 to +429
- edm4hep::CalorimeterHit from // reference to the reconstructed hit
- edm4hep::SimCalorimeterHit to // reference to the simulated hit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- edm4hep::CalorimeterHit from // reference to the reconstructed hit
- edm4hep::SimCalorimeterHit to // reference to the simulated hit
- edm4hep::CalorimeterHit to // reference to the reconstructed hit
- edm4hep::SimCalorimeterHit from // reference to the simulated hit

The name of the datatype suggests that the MC part is first, so I think it is more intuitive for it to be the "from" part of the association, right? A similar argument can be made for some of the other associations as well.

@hegner
Copy link
Contributor

hegner commented Jul 4, 2023

Outdated and decided not to pick up

@hegner hegner closed this Jul 4, 2023
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