Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add a LinkNavigator utility #646

Merged
merged 15 commits into from
Dec 5, 2024
Merged

Add a LinkNavigator utility #646

merged 15 commits into from
Dec 5, 2024

Conversation

tmadlener
Copy link
Collaborator

@tmadlener tmadlener commented Jul 24, 2024

BEGINRELEASENOTES

  • Add a LinkNavigator utility class that facilitates the lookup of linked objects

ENDRELEASENOTES

The main working principle is:

// From MC to reco particles
auto& mc2recoLinks = event.get<RecoMCParticleLinkCollection>("MCRecoTruthLink");
auto mc2reco = LinkNavigator{mc2recoLinks};

auto& mcps = event.get<MCParticle>("MCParticles");
for (const auto p : mcps) {
  const auto recos = mc2reco.getLinked(p);
}

recos in this case is a std::vector<WeightedObject<ReconstructedParticle>>, which effectively just couples an object and a relation weight together. The foreseen usage is something like this:

for (const auto p : mcps) {
  for (const auto& [reco, weight] : mc2reco.getLinked(p)) {
    // do something with reco and the weight
  }
}

@tmadlener tmadlener changed the title Add an AssociationNavigator utility Add a LinkNavigator utility Jul 31, 2024
@tmadlener tmadlener force-pushed the association-nav branch 2 times, most recently from f1fc3ce to ed26399 Compare October 15, 2024 17:15
@tmadlener
Copy link
Collaborator Author

This is now also ready for review and I would like to merge it before the next tag.

doc/links.md Outdated
const auto linkedRecs = linkNavigator.getLinked(mcParticle);
// If you want to make a point about the direction use getLinked{From,To}
// This is also necessary if the From and To type in the link collection are the same
const auto linkedMCs = linkNavigator.getLinkedFrom(recoParticle);
Copy link
Member

Choose a reason for hiding this comment

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

I am confused about what the getLinkedFrom returns. It returns the "To" entries of the links, right?
But names like getToLinkedFromFrom and getFromLinkedFromTo also sound a bit ridiculous?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly. I started the naming for these from the arguments. Maybe we can drop that detail from the documentation as well, as for the majority of links, I assume the templated version will do correct thing in any case.

Alternatively, we could also introduce a two argument version that takes the direction as some enum?

Copy link
Member

Choose a reason for hiding this comment

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

I think a two argument version of getLinked with an enum sounds less confusing and more explicit than getLinkedFrom.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have introduced a version with an overload set that takes a second argument to decide on the direction. Is Lookup{From,To} clear enough?

Copy link
Member

Choose a reason for hiding this comment

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

I proposed some word changes to avoid re-using to and from in the documentation strings.

I think instead of Lookup Return might be more explicit.

getLinked(recoParticle, ReturnFrom);

makes it more obvious that we return from objects?

return getLinkedTo(object);
}

/// Get all the objects and weights that are linked to the passed object
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
/// Get all the objects and weights that are linked to the passed object
/// Get all the objects and weights that are linked from the passed object

(?)

Copy link
Member

Choose a reason for hiding this comment

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

No?
Rather

Suggested change
/// Get all the objects and weights that are linked to the passed object
/// Get all the *To* objects and weights that have links with the passed object

LinkNavigator& operator=(LinkNavigator&&) = default;
~LinkNavigator() = default;

/// Get all the objects and weights that are linked to the passed object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get all the objects and weights that are linked to the passed object
/// Get all the *From* objects and weights that have links with the passed object

Now all overloads have the same name, and selection is done via a tag
argument.
Comment on lines 36 to 37
/// Tag variable to select the lookup of *From* objects that are linked from a
/// *To* object in podio::LinkNavigator::getLinked
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Tag variable to select the lookup of *From* objects that are linked from a
/// *To* object in podio::LinkNavigator::getLinked
/// Tag variable to select the lookup of *From* objects that have links with the given
/// *To* object in podio::LinkNavigator::getLinked

Comment on lines 39 to 40
/// Tag variable to select the lookup of *To* objects that are linked from a
/// *From* object in podio::LinkNavigator::getLinked
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Tag variable to select the lookup of *To* objects that are linked from a
/// *From* object in podio::LinkNavigator::getLinked
/// Tag variable to select the lookup of *To* objects that have links with the given
/// *From* object in podio::LinkNavigator::getLinked

@tmadlener
Copy link
Collaborator Author

Thanks for the wording suggestions. Unless I missed some instances (which is not unlikely), I think it should now all be more or less self-consistent.

Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>
const auto linkedMCs = linkNavigator.getLinked(recoParticle, podio::ReturnTo);
```

The return type of all methods is a `std::vector<WeightedObject>`, where the
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
The return type of all methods is a `std::vector<WeightedObject>`, where the
The return type of all methods is a `std::vector<podio::detail::links::WeightedObject>`, where the

With the implicit question whether we should lift this object out of the (implicitly private) detail namespace, since it is in principle user facing.

@tmadlener tmadlener merged commit c97bdf1 into master Dec 5, 2024
18 checks passed
@tmadlener tmadlener deleted the association-nav branch December 5, 2024 08:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants