-
Notifications
You must be signed in to change notification settings - Fork 26
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
Don't use GaudiAlg #172
Don't use GaudiAlg #172
Conversation
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.
Looks good, but I am not sure we should keep all the "old" examples for creating example data, because I don't think our DataHandle
is thread safe enough to just b marked mutable
.
Alternative proposal (changes in the last commit): Remove the UniquePtr test which isn't doing anything than the other tests are not doing and set Cardinality to 1 which is how algorithms that can't run in parallel are set, for example: https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiKernel/include/GaudiKernel/Algorithm.h#L24 Having some algorithms around is good because not everything will be functional and it also allows to test if they can be mixed together. |
If there aren't any more complaints I'll merge this today. I think the plan is to remove GaudiAlg for the version 40 of Gaudi so there is time but there are a few repos that have to be changed |
I think this looks good. One thing that is not entirely clear to me: Is the |
I'm not sure I have seen that being used by an algorithm. There is an event number that could be used for seeding I guess but we have that information in the headers |
BEGINRELEASENOTES
ENDRELEASENOTES
This can also serve as example for other repos, basically adding a
const EventContext&
in the arguments toexecute
and makingexecute
const and changingGaudiAlgorithm
toGaudi::Algorithm
. Headers change too:GaudiAlg/GaudiAlgorithm.h
toGaudi/Algorithm.h