-
Notifications
You must be signed in to change notification settings - Fork 27
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
Variadic Podio Collections cannot be passed directly into the algorithms defined through the algorithms interface #1401
Comments
I'm not planning on changing this in JANA, and here is my rationale:
a. All of the requested collections were found. In this case, the not_null is redundant because if any collection cannot be found, the factory has to except. (You can credibly argue that the not_null is still valuable due to ergonomics and/or defending against bad programmers and/or the principle of making invalid states unrepresentable and/or the trend of making things monadic, but I've also thought of these things and I'll happily debate you in private.) b. Some of the requested collections were found. In this case, we lose important information about which of the requested collections were found because Podio doesn't store this as part of the collection itself (nor do the old JANA vector<JObject*> collections, for whatever it's worth). The correct thing to do if we want the algorithm execution to proceed is to return a vector of optionals.
To be fair, the only reason we end up with 16 or 32 or 64 templates is because we want to keep having a convenient
On a personal note, I think that So for now, JANA2 is going to let the collections be raw pointers. If the user sets is_optional=false, those pointers won't be null. If they set is_optional=true, they will have to check for nullity themselves. Problem solved. |
Thanks for the detailed explanation. The details and handling of different pointers in C++ is still a bit new to me. Of course this isn't a pressing issue and maybe should them be changed in "algorithms" somewhere down the line just to remove this current quirk that's required. |
Yeah, I also want to iron out that very awkward little quirk. We'll get there! |
Currently in order to pass a variadic collection as an argument into the algorithm it needs to be recast from VariadicPodioInput/VariadicPodioOutput to an e.g. std::vector<gsl::not_nulledm4hep::TrackerHitCollection*>
Presumably this really needs to be tackled in either JANA2 or Algorithms rather than EICRecon.
Examples:
https://github.com/eic/EICrecon/blob/main/src/global/pid/MergeTrack_factory.h#L46
https://github.com/eic/EICrecon/blob/main/src/factories/meta/CollectionCollector_factory.h#L31
The text was updated successfully, but these errors were encountered: