-
Notifications
You must be signed in to change notification settings - Fork 13
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
Transformer to create tracks from gen particles #21
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.
Some comments. I didn't run it, but it looks fine
if not os.path.isfile("ddsim_output_edm4hep.root"): | ||
os.system("ddsim --enableGun --gun.distribution uniform --gun.energy '10*GeV' --gun.particle e- --numberOfEvents 100 --outputFile ddsim_output_edm4hep.root --random.enableEventSeed --random.seed 42 --compactFile $K4GEO/FCCee/IDEA/compact/IDEA_o1_v02/IDEA_o1_v02.xml") |
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.
Here I'd say it's preferrable to print a message if the file is not there, but always running forcefully ddsim is maybe a bit too much. Note that this will run as soon as the file is processed, before anything related to Gaudi has been loaded or checked.
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.
Hi Juan, thanks for the comments! Not sure I understand this one though. This line is just for convenience (and for tests). So that I can run/test the genTracking just calling one script
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.
A couple more comments:
- In the steering file it would be good to get rid of
PodioInput
andPodioOutput
and useIOSvc
like in https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/options/ExampleFunctionalFile.py#L28. It should work withPodioInput
andPodioOutput
but if you ever want to run multithreaded it won't - For histogramming I think that works fine, but since a few days ago there is an example in k4FWCore: https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/src/components/ExampleFunctionalTransformerHist.cpp and https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/options/ExampleFunctionalTransformerHist.py where in the cpp file you only need to fill the histogram and the exporting gets done by the
RootHistSvc
.
Is this really wanted/important? I need to access the TH1 to add the overflow bin |
Done |
@jmcarcell the test related to this PR passes (I removed Event Counter). Shall we handle the other tests in another PR? |
In general a different PR is better because then people can look back and understand why the change was done, otherwise they will find this PR... |
BEGINRELEASENOTES
ENDRELEASENOTES