-
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
Add a Gaudi Algorithm filling EventHeader eventNumber and runNumber #133
Conversation
I think this is very useful. One comment on the naming: Could that reflect that this creates an EventHeader collection? I.e. Also this is most likely useful mainly for debugging / developments, as in "proper workflows" something outside the framework should probably populate this (?).
Making this thread safe from a technical point of view could probably be done with either an atomic or a mutex/lock. However, there is a more conceptual question of what this "event counter" even means in a multi threaded context. At least the expectation that the event numbers are sequential has to be dropped.
At least ddsim fills this information now from the input generator file, since AIDASoft/DD4hep#1059
This is probably more related to EDM4hep itself. |
Could you also resolve the conflict in the CMakeLists file. From a quick glance it should be fairly straight forward and it should be possible to keep everything. |
Hi @tmadlener, thanks for taking a look. Is it ok to have the "Merge branch main" commit or do you want me to rebase?
Done
Why would this have to live outside of the framework? I anticipated that a job submitter could interact with this algorithm by setting the |
Depending on when you enter the framework, you already have an event (and a run number), e.g. from the generator / simulation or the DAQ. So the "normal user" doesn't have to populate this information, unless they start from the vacuum effectively. Obviously, if you start from empty events or if you have to add this after the fact, this is very useful.
That is OK. We will squash in any case when merging. |
Ok thanks, then that would probably be another algorithm retrieving the info from external source. |
Any further comment on this PR? |
Not from my side - thanks for including this feature! |
for developing: install somehow pre-commit, and |
Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>
Done |
# limitations under the License. | ||
# | ||
from Gaudi.Configuration import * | ||
|
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.
Stupid question (and I should know this), does this produce the expected output file if we don't use our Key4hep data service (k4DataSvc
)?
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.
If I understand you question correctly: this steering file does not produce any output file.
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.
Ah yes, you are obviously right. Could you make it produce an output file by adding the necessary algorithm. Mainly to make everyone see how a complete example looks like and so that they then also have an output file that they can look at after running the tests.
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.
Sure, here it is
That is indeed very useful. |
I open this PR so that people can already use it. @ebrondol let me know if that fits your needs or not.
Some open questions:
edm4hep::EventHeader
also holds the MC event weight. This should be filled by retrieving information from the MC generator (e.g. aMC@NLO). IMHO the eventNumber filling should not be tight to MC generation. We would then have to create a second eventHeader collection for the MC weights since it is const (or is there a workaround?) which is not nice. Maybe we should separate the MC weight from the eventNumber in edm4ehp?BEGINRELEASENOTES
ENDRELEASENOTES