-
Notifications
You must be signed in to change notification settings - Fork 482
Default TPC loopers in o2-sim #14851
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: dev
Are you sure you want to change the base?
Conversation
Fixed arrays in TPCLoopersParams and implemented comments
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
Please consider the following formatting changes to AliceO2Group#14851
|
Error while checking build/O2/fullCI_slc9 for 76f9996 at 2025-12-03 15:26: Full log here. |
Generators/CMakeLists.txt
Outdated
| src/GeneratorTParticleParam.cxx | ||
| src/GeneratorService.cxx | ||
| src/FlowMapper.cxx | ||
| $<$<BOOL:${onnxruntime_FOUND}>:src/TPCLoopers.cxx> |
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 think we always have onnxruntime. It's specified as requirement in alidist/o2.sh; So I would not make it a conditional compile and this will also make the code cleaner (less #ifdefs)
| #include "FairGenerator.h" | ||
| #include "TParticle.h" | ||
| #include "Generators/Trigger.h" | ||
| #include "CCDB/BasicCCDBManager.h" |
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.
only include headers in headers if it cannot be avoided. Better to include in source file.
|
|
||
| #ifdef GENERATORS_WITH_TPCLOOPERS | ||
| // Loopers generator instance | ||
| std::unique_ptr<o2::eventgen::GenTPCLoopers> mLoopersGen = nullptr; |
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 general, I would suggest to be (consistently) more specific. Sometimes we say "TPCLoopers" ... sometimes just "mLoopers". I think it would be good to always put TPC in the name. So mLoopersGen --> mTPCLoopersGen and the same for the functions.
| #include "TDatabasePDG.h" | ||
| #include <SimulationDataFormat/DigitizationContext.h> | ||
| #include <SimulationDataFormat/ParticleStatus.h> | ||
| #include "SimulationDataFormat/MCGenProperties.h" |
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 reduce this include list to a minimum and prefer inclusion where they are used (in the source).
| { | ||
|
|
||
| #ifdef GENERATORS_WITH_TPCLOOPERS | ||
| class GenTPCLoopers |
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 small doxygen style class description would be good
| // Random number generator | ||
| TRandom3 mRandGen; | ||
| // Masses of the electrons and positrons | ||
| TDatabasePDG* mPDG = TDatabasePDG::Instance(); |
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.
do we need this pointer as a member function?
| // Masses of the electrons and positrons | ||
| TDatabasePDG* mPDG = TDatabasePDG::Instance(); | ||
| double mMass_e = mPDG->GetParticle(11)->Mass(); | ||
| double mMass_p = mPDG->GetParticle(-11)->Mass(); |
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.
same here. These are rather constants. Probably no need to have them as class member variables.
| std::string gauss = "${O2_ROOT}/share/Generators/egconfig/gaussian_params.csv"; // file with Gaussian parameters | ||
| std::string scaler_pair = "${O2_ROOT}/share/Generators/egconfig/ScalerPairParams.json"; // file with scaler parameters for e+e- pair production | ||
| std::string scaler_compton = "${O2_ROOT}/share/Generators/egconfig/ScalerComptonParams.json"; // file with scaler parameters for Compton scattering | ||
| std::string nclxrate = "ccdb://Users/m/mgiacalo/ClustersTrackRatio"; // file with clusters/rate information per orbit |
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.
Preferably avoid user-paths. (We might put it under TPC detector). What is this object in any case?
Generators/src/TPCLoopers.cxx
Outdated
|
|
||
| // Generate a latent vector (z) | ||
| std::vector<float> z(100); | ||
| for (auto& v : z) |
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 add {} bodies here and in a few other places (FullCI is failing).
Generators/src/TPCLoopers.cxx
Outdated
| return true; | ||
| } | ||
|
|
||
| Bool_t GenTPCLoopers::generateEvent(double& time_limit) |
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.
do you need double& (reference) ?
Generators/src/TPCLoopers.cxx
Outdated
| return gaussValue; | ||
| } | ||
|
|
||
| void GenTPCLoopers::SetNLoopers(unsigned int& nsig_pair, unsigned int& nsig_compton) |
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.
prefer passing constant pod args as non-reference
Generators/src/TPCLoopers.cxx
Outdated
| } | ||
| } | ||
|
|
||
| void GenTPCLoopers::SetMultiplier(std::array<float, 2>& mult) |
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.
const std::array & ?
Generators/src/TPCLoopers.cxx
Outdated
| } | ||
| } | ||
|
|
||
| void GenTPCLoopers::setFlatGas(Bool_t& flat, const Int_t& number = -1, const Int_t& nloopers_orbit = -1) |
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.
dito
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.
default args are normally only specific in the function declaration
|
I implemented all the comments and retested the code. |
TPC loopers are currently injected via an O2DPG external generator by creating a cocktail. This development feeds them automatically in case the TPC detector is used and transported. A collisioncontext.root file is mandatory, otherwise the loopers will be automatically turned off. In addition the GenTPCLoopers.loopersVeto parameter has been introduced to forcefully turn off the injection. This PR is linked to AliceO2Group/O2DPG#2191, in which the collision system is automatically fed to the sgnsim step