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

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 Show resolved Hide resolved
doc/links.md Outdated Show resolved Hide resolved
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.

include/podio/LinkNavigator.h Outdated Show resolved Hide resolved
include/podio/LinkNavigator.h Outdated Show resolved Hide resolved
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
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.

2 participants