Gamma conversion generator #1195
Conversation
|
Hi @sdifalco,
which require these tests: build. @Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main. The following users requested to be notified about changes to these packages: ⌛ The following tests have been triggered for a0d1604: build (Build queue is empty) |
|
☔ The build is failing at a0d1604.
N.B. These results were obtained from a build of this Pull Request at a0d1604 after being merged into the base branch at 635a10d. For more information, please check the job page here. |
|
Adding a comment that I sent to Stefano directly in response to a question from him. it may be of general interest.
|
|
📝 The HEAD of |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 293938e: build (Build queue is empty) |
|
☀️ The build tests passed at 293938e.
N.B. These results were obtained from a build of this Pull Request at 293938e after being merged into the base branch at 666d876. For more information, please check the job page here. |
| } | ||
| //const auto& vols = PhysicalVolumeInfos->at(gammapart->simStage()); | ||
| /* | ||
| PhysicalVolumeInfoSingleStage const* vols = &PhysicalVolumeInfos->at(gammapart->simStage()); |
There was a problem hiding this comment.
Please either remove the commented out code or put it behind a verbosity level. The one exception to this request is if the commented out code is something in development.
| } | ||
| G4Material*stopMat = findMaterialOrThrow((G4String const& )endInfo.materialName()); | ||
| */ | ||
| G4Material*stopMat = findMaterialOrThrow((G4String const& )stopMaterial_); |
There was a problem hiding this comment.
I am 99% sure that geometry and materials are defined at beginRun time. So you can move the lookup of the element fractions to beginRun and save only the information you need.
There was a problem hiding this comment.
geometry readout moved to beginrun
There was a problem hiding this comment.
unnecessary PhysVol collection unpacking deleted
| } | ||
|
|
||
| // Take the converted gamma as SimParticle | ||
| const auto gammapart = gammas.at(eng_.operator unsigned int() % gammas.size()); |
There was a problem hiding this comment.
This is not guaranteed to uniformly sample the set of gammas. The effect is negligible. Did you do it this way since it's advertised as being faster than randFlat_,fireInt( gammas.size());
There was a problem hiding this comment.
I have inherited this from Michael's code. If you think it's better I can use randFlat.
There was a problem hiding this comment.
changing to what is suggested
|
@FNALbuild run build test |
|
@sdifalco please let me know when your are finished with this set of commits and I will rerun the CI |
|
⌛ The following tests have been triggered for 71d94e3: build (Build queue has 1 jobs) |
|
I have deleted the commented code I have found. |
|
Now I have warnings from Thanks |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 71e9a7b: build (Build queue has 1 jobs) |
|
Hi Stefano,
The memory leak is in the new used to create the object of type GammaPairConversionSpectrum that is pointed to by the variable spectrum_ . It is never deleted. While it’s not a big memory leak it’s easy to fix.
Rob
From: sdifalco ***@***.***>
Reply-To: Mu2e/Offline ***@***.***>
Date: Thursday, February 22, 2024 at 4:45 PM
To: Mu2e/Offline ***@***.***>
Cc: Rob Kutschke ***@***.***>, Team mention ***@***.***>
Subject: Re: [Mu2e/Offline] Gamma conversion generator (PR #1195)
[EXTERNAL] – This message is from an external sender
I have deleted the commented code I have found.
The map is now private (no need for a set function, a get function is enough for what I see)
I don't see the requested change inline in the code to avoid memory leakage: was this one?
Thanks
—
Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1195-23issuecomment-2D1960455342&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=Y66woKf-yZpMiZpN1C9_C8yHOj3hysbsNEIB_fzl3G-zN8zWlBpjPkF_zw0-lpD7&s=zgGIKK5Jy_IcIguwC6bdUTKgp3M_d9biAoAMaRvQg3Q&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABHY5ZS43G42TVCT3RSIAV3YU7C6XAVCNFSM6AAAAABDOLH6N6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGQ2TKMZUGI&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=Y66woKf-yZpMiZpN1C9_C8yHOj3hysbsNEIB_fzl3G-zN8zWlBpjPkF_zw0-lpD7&s=oqmXDQGNmGNryVy19PTjVZ8qR8FcyAPP0BhvKwNKrJQ&e=>.
You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>
|
Let me check on thing - it will take 5 minutes. |
|
.... I think it is suggesting: constexpr std::array<double,3> sfa1{42.038,-8.29,0.958}; And a small detail. Where you have: If you write that as it gives the compiler more information. It explicitly says that FZ_const is a compile time constant and the optimizer may treat it as such. The way that your wrote it, it's very likely the optimizer will figure that out but it has to do some work to do so. It's not a big deal but it is an example of just do it right all of the time so that when it matters you will just do it right. |
|
☀️ The build tests passed at 71e9a7b.
N.B. These results were obtained from a build of this Pull Request at 71e9a7b after being merged into the base branch at 07adcaa. For more information, please check the job page here. |
|
You only need this much:
|
|
I had to add static in front of constexpr to compile |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for a6ea179: build (Build queue is empty) |
Thanks - I forgot about the static. Good catch. It's needed for constexpr data members but it is not needed for constexpr stack variables in function scope. Presuming the CI goes well, I will merge tonight. |
|
☀️ The build tests passed at a6ea179.
N.B. These results were obtained from a build of this Pull Request at a6ea179 after being merged into the base branch at 07adcaa. For more information, please check the job page here. |
New complete generator for gamma conversion based on previous Michael Makenzie's module in Su2020 repository. As agreed with Rob the generator will go in the Mu2eG4 directory instead of the usual EventGenerator because it uses some G4 libraries. The SConscript in Mu2eG4/src also needs a fix: mu2e_DataProducts needs to be added in helper.make_plugins