-
Notifications
You must be signed in to change notification settings - Fork 135
SST optimisation #743
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
base: master
Are you sure you want to change the base?
SST optimisation #743
Conversation
olantwin
left a comment
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 first impressions.
If we want to merge this eventually, MyGenerator definitely needs a sensible name.
macro/update_from_file.py
Outdated
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.
In addition to the problem of the config format, I don't like how this depends on being able to modify global state.
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.
Could you please tell more on what do you mean by global state. I've re-imlemented changing parameters using a flag, is it ok?
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'd prefer functions without side effects, to not surprise users.
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.
Please make sure that the cmakelint and cmake-format pre-commit hooks run cleanly on this.
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.
Clean up the commented code, update comments etc. which were copy-pasted from the strawtubes
shipgen/CMakeLists.txt
Outdated
| ${CMAKE_SOURCE_DIR}/shipdata | ||
| ${CMAKE_SOURCE_DIR}/veto | ||
| ${genfit2_INCDIR} | ||
| ${TPYTHIA6_INCLUDE_DIR} |
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.
What does your PR have to do with Pythia6?
shipgen/NtupleGenerator.cxx
Outdated
| cout << "-E NtupleGenerator: Error opening the Signal file" << fileName << endl; | ||
| } | ||
| fTree = (TTree *)fInputFile->Get("ntuple"); | ||
| fTree = (TTree *)fInputFile->Get("pythia8-Geant4"); |
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 C-style casts
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.
What is this 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.
example file for MyGenerator; to show a needed structure of the ttree
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.
Please remove it. You can put an example file on EOS, and maybe mention it in the documentation of the generator, but example ROOT files should not be tracked in git.
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.
What's the purpose of this 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.
It produces file with variables that are necessary for MyGenerator only.
So only vertex positions, momentum and pdg code stored in strawdetector
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 this is not of sufficient general interest to include in the main framework.
|
Also, please fix the merge conflicts, so that we can run the automatic checks. |
| parser.add_argument("--SND", dest="SND", help="Activate SND.", action='store_true') | ||
| parser.add_argument("--noSND", dest="SND", help="Deactivate SND. NOOP, as it currently defaults to off.", action='store_false') | ||
| #parser.add_argument("--psdz", dest="psdz", help="Set Prestrawdetector Z position", required=False, default=2586., type=float) | ||
| parser.add_argument("--decouple", dest="decouple", help="Decoupling flag", required=False, default=False, action='store_true') |
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.
This needs a better name and help message
macro/run_simScript.py
Outdated
| ) | ||
| parser.add_argument("--SND", dest="SND", help="Activate SND.", action='store_true') | ||
| parser.add_argument("--noSND", dest="SND", help="Deactivate SND. NOOP, as it currently defaults to off.", action='store_false') | ||
| #parser.add_argument("--psdz", dest="psdz", help="Set Prestrawdetector Z position", required=False, default=2586., type=float) |
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.
Why is this commented out?
macro/run_simScript.py
Outdated
| ) | ||
|
|
||
| ship_geo.decouple = options.decouple | ||
| ship_geo.psd=2586.0 |
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.
This needs a better name and comments to explain what these are, especially if they are set at the top level!
|
@AnalitikGnid Any updates on this? Some responses to the various questions and comments would be appreciated as well. |
|
@AnalitikGnid Any updates? |
1bea498 to
20fb5d2
Compare
8a6e012 to
2ced8d1
Compare
Useful tools for SST decoupling and optimisation that could be used further
Added:
prestrawdetector— a thin air plane that can be placed anywhere in the simulation to store events.MyGenerator— a new generator module that injects events from a custom input file