Mu2e / Offline

Offline software for the Mu2e experiment
Apache License 2.0
8 stars 81 forks source link

Add geantino #1271

Closed AndrewEdmonds11 closed 4 months ago

AndrewEdmonds11 commented 4 months ago

This PR adds in the geantino. For some reason the PDGcode number on its own doesn't work and so I had to add an else block to Mu2eG4PrimaryGeneratorAction to get it by name.

FNALbuild commented 4 months ago

Hi @AndrewEdmonds11, You have proposed changes to files in these packages:

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

The following users requested to be notified about changes to these packages: @resnegfk

:hourglass: The following tests have been triggered for 6e751334d397894baa05188264babe13a4d06771: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild commented 4 months ago

:sunny: The build tests passed at 6e751334d397894baa05188264babe13a4d06771.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 6e751334d397894baa05188264babe13a4d06771 at a8f0771cf53c236b29f4bdaa9b2ebdb58483604d
build (prof) :white_check_mark: Log file. Build time: 11 min 08 sec
ceSimReco :white_check_mark: Log file.
g4test_03MT :white_check_mark: Log file.
transportOnly :white_check_mark: Log file.
POT :white_check_mark: Log file.
g4study :white_check_mark: Log file.
cosmicSimReco :white_check_mark: Log file.
cosmicOffSpill :white_check_mark: Log file.
ceSteps :white_check_mark: Log file.
ceDigi :white_check_mark: Log file.
muDauSteps :white_check_mark: Log file.
ceMix :white_check_mark: Log file.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :large_orange_diamond: TODO (0) FIXME (1) in 3 files
clang-tidy :large_orange_diamond: 0 errors 55 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 6e751334d397894baa05188264babe13a4d06771 after being merged into the base branch at a8f0771cf53c236b29f4bdaa9b2ebdb58483604d.

For more information, please check the job page here. Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

brownd1978 commented 4 months ago

I added Richie as PDG expert to make sure this is consistent with future PDG group plans.

FNALbuild commented 4 months ago

:memo: The HEAD of main has changed to 18b68d924590dcfd16fd7874ee914e76b43eca67. Tests are now out of date.

kutschke commented 4 months ago

@bonventre and @rlcee I hope you both enjoyed your long weekend. A gentle reminder that reviewing this PR is in your inbox.

Richie: is this fix consistent with PDG future plans - for example has PDG discussed adding a code for geantinos? If not, that's OK - we just want to future proof if we can.

Ray: is this consistent with your design of the ParticleDataList?

kutschke commented 4 months ago

@FNALbuild run build test

FNALbuild commented 4 months ago

:hourglass: The following tests have been triggered for 6e751334d397894baa05188264babe13a4d06771: build (Build queue is empty)

FNALbuild commented 4 months ago

:sunny: The build tests passed at 6e751334d397894baa05188264babe13a4d06771.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 6e751334d397894baa05188264babe13a4d06771 at 18b68d924590dcfd16fd7874ee914e76b43eca67
build (prof) :white_check_mark: Log file. Build time: 11 min 08 sec
ceSimReco :white_check_mark: Log file.
g4test_03MT :white_check_mark: Log file.
transportOnly :white_check_mark: Log file.
POT :white_check_mark: Log file.
g4study :white_check_mark: Log file.
cosmicSimReco :white_check_mark: Log file.
cosmicOffSpill :white_check_mark: Log file.
ceSteps :white_check_mark: Log file.
ceDigi :white_check_mark: Log file.
muDauSteps :white_check_mark: Log file.
ceMix :white_check_mark: Log file.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :large_orange_diamond: TODO (0) FIXME (1) in 3 files
clang-tidy :large_orange_diamond: 0 errors 55 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 6e751334d397894baa05188264babe13a4d06771 after being merged into the base branch at 18b68d924590dcfd16fd7874ee914e76b43eca67.

For more information, please check the job page here. Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

bonventre commented 4 months ago

PDG does have some nonzero codes reserved for generators etc. (see https://pdg.lbl.gov/2023/reviews/rpp2023-rev-monte-carlo-numbering.pdf point 10. They do not use 0 in case it is returned as an error state somewhere.

brownd1978 commented 4 months ago

It looks like 998 would be a good choice. I searched for that in Geant4 repo and didn't find any (non-trivial) instances.

On Tue, May 28, 2024 at 12:56 PM Richie Bonventre @.***> wrote:

PDG does have some nonzero codes reserved for generators etc. (see https://pdg.lbl.gov/2023/reviews/rpp2023-rev-monte-carlo-numbering.pdf point 10. They do not use 0 in case it is returned as an error state somewhere.

— Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/pull/1271#issuecomment-2136003782, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH573467CW4C42GMXGV7TZETOOBAVCNFSM6AAAAABIF6L4DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZWGAYDGNZYGI . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

-- David Brown @.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

kutschke commented 4 months ago

@resnegfk When you have a chance, please look at Offline PR #1271 . We want to assign a PDGCode ( in our PDGCode system ) to a geantino. This is 100% within Mu2e code and does not affect G4 in any way. The PR as it exists now uses 0. Richie pointed us to the authoritative PDG document which says that codes 998 and 999 are reserved for G4 tracking. Dave suggest we use 998. Do you see any issues with this choice? I think we are Ok since we do not count on any internal G4 numbering scheme, which look up the geantino G4Particle by name.

kutschke commented 4 months ago

Thanks everyone. If Krzysztof signs off on 998 then Andy should make the change and we are done.

rlcee commented 4 months ago

Doesn't geant declare the geantino to be id=0? And that is copied into the SimParticle? How do you get around that? (Leaving aside for the moment that the geant particle list doesn't accept id=0. When you ask geant particle list for "geantino", does it find it and report id=0?).

kutschke commented 4 months ago

Doesn't geant declare the geantino to be id=0? And that is copied into the SimParticle? How do you get around that? (Leaving aside for the moment that the geant particle list doesn't accept id=0. When you ask geant particle list for "geantino", does it find it and report id=0?).

Duh. @AndrewEdmonds11 Please run this test. When you get the G4ParticleDefinition for the geantino, call it's GetPDGEncoding() member function. Unless there is a conflict, that's the number to use.

brownd1978 commented 4 months ago

According to GitHub, that number is 0: https://github.com/Geant4/geant4/blob/dda54bbdcfe0cc76177eaa03a657592aac0b1eb8/source/particles/bosons/src/G4ChargedGeantino.cc#L66 so there's a mismatch between the pdg and g4 docs. I agree going with the G4 answer is the best.

On Tue, May 28, 2024 at 4:06 PM Rob Kutschke @.***> wrote:

Doesn't geant declare the geantino to be id=0? And that is copied into the SimParticle? How do you get around that? (Leaving aside for the moment that the geant particle list doesn't accept id=0. When you ask geant particle list for "geantino", does it find it and report id=0?).

Duh. @AndrewEdmonds11 https://github.com/AndrewEdmonds11 Please run this test. When you get the G4ParticleDefinition for the geantino, call it's GetPDGEncoding() member function. Unless there is a conflict, that's the number to use.

— Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/pull/1271#issuecomment-2136245919, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH57324ITXXXTLDJJOKEDZEUEYRAVCNFSM6AAAAABIF6L4DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZWGI2DKOJRHE . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

-- David Brown @.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

kutschke commented 4 months ago

Thanks Dave. Andy's original choice of 0 stands.

@AndrewEdmonds11 - no need to do the test i requested.

About Richie's comments about PDG reserving 0 for a possible error state. Our code has an explicit representations for an unknown value and explicit notions of validity, implemented in the EnumToStringSpace::isValid static member function. So I think we have that covered consisently in our own code base. If PDG does release code that we choose to adopt, I expect tehre wi be just one of many adaptions we will need to make.

Thanks everyone for your input. I am merging now.