-
Notifications
You must be signed in to change notification settings - Fork 60
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
Conversation
3f580b6
to
ed4bc4e
Compare
f1fc3ce
to
ed26399
Compare
ed26399
to
af1e4f8
Compare
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
include/podio/LinkNavigator.h
Outdated
return getLinkedTo(object); | ||
} | ||
|
||
/// Get all the objects and weights that are linked to the passed object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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 |
(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No?
Rather
/// 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 |
include/podio/LinkNavigator.h
Outdated
LinkNavigator& operator=(LinkNavigator&&) = default; | ||
~LinkNavigator() = default; | ||
|
||
/// Get all the objects and weights that are linked to the passed object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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.
include/podio/LinkNavigator.h
Outdated
/// Tag variable to select the lookup of *From* objects that are linked from a | ||
/// *To* object in podio::LinkNavigator::getLinked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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 |
include/podio/LinkNavigator.h
Outdated
/// Tag variable to select the lookup of *To* objects that are linked from a | ||
/// *From* object in podio::LinkNavigator::getLinked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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 |
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 <[email protected]>
const auto linkedMCs = linkNavigator.getLinked(recoParticle, podio::ReturnTo); | ||
``` | ||
|
||
The return type of all methods is a `std::vector<WeightedObject>`, where the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
BEGINRELEASENOTES
LinkNavigator
utility class that facilitates the lookup of linked objectsENDRELEASENOTES
The main working principle is:
recos
in this case is astd::vector<WeightedObject<ReconstructedParticle>>
, which effectively just couples an object and a relation weight together. The foreseen usage is something like this:std::is_same_v<FromT, ToT>