Skip to content
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

Import muon pair production sampling table from Geant4 #1419

Merged
merged 9 commits into from
Sep 24, 2024

Conversation

amandalund
Copy link
Contributor

This imports the table used for sampling the electron-positron pair energy from the Geant4 muon pair production model.

@amandalund amandalund added enhancement New feature or request external Dependencies and framework-oriented features labels Sep 21, 2024
Comment on lines +470 to +473
* \todo Prior to version 11.1.0, Geant4 used the \c G4BetheBlochModel for muon
* ionization between 200 keV and 1 GeV and the \c G4MuBetheBlochModel above 1
* GeV. Since version 11.1.0, the \c G4MuBetheBlochModel is used for all
* energies above 200 keV.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One slight complication is that different Geant4 versions use different models for muon ionization...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the newer model available in both? Or should we just default to the same models available in the selected version of G4?

@sethrj
Copy link
Member

sethrj commented Sep 23, 2024

Ummmm....

155/265 Test #157: celeritas/ext/GeantImporter:FourSteelSlabs* .....................................***Exception: SegFault  0.39 sec
Celeritas version 0.5.0-dev.277+20f1e85
Note: Google Test filter = FourSteelSlabs*
[==========] Running 17 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 17 tests from FourSteelSlabsEmStandard
[ RUN      ] FourSteelSlabsEmStandard.em_particles

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Nice work.

Comment on lines +470 to +473
* \todo Prior to version 11.1.0, Geant4 used the \c G4BetheBlochModel for muon
* ionization between 200 keV and 1 GeV and the \c G4MuBetheBlochModel above 1
* GeV. Since version 11.1.0, the \c G4MuBetheBlochModel is used for all
* energies above 200 keV.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the newer model available in both? Or should we just default to the same models available in the selected version of G4?

@amandalund
Copy link
Contributor Author

Yeah I'm not sure what's up with that test, and I don't have Celeritas built with Geant4 11.2 yet...

@sethrj
Copy link
Member

sethrj commented Sep 23, 2024

The good news is it's reproducible on my machine with 11.2.1; the bad news is that GetElement2DData returns garbage for Z=12... perhaps the semantics of that call have changed?

@amandalund
Copy link
Contributor Author

Ah excellent, thanks @sethrj! I'll look into it...

@sethrj
Copy link
Member

sethrj commented Sep 23, 2024

Wait wait wait, it's not too late, but we almost missed the opportunity to call that Element Table a MuPPET!

@amandalund
Copy link
Contributor Author

Ok it looks like in v11.2+ G4ElementData can be constructed with the maximum number of elements (which should be greater than the largest Z). G4MuPairProductionModel now uses this constructor, but calls it with NZDATPAIR, which is the size of the Z array, which is 5. It looks like there should be some kind of error when initializing the data though for Z >= 5...

@sethrj
Copy link
Member

sethrj commented Sep 23, 2024

@amandalund Yeah, so the ElementTable is designed to have the index be the element ID, but the MuPPET is fixed at size 5 (an implementation detail) and those are just indices (index of ZDATPAIR) rather than Z number (value in ZDATPAIR). There appears to be no way to get the size (the private data member maxNumElm). Also it seems to use the Z number to access the muppet in some places, and uses the index to set it. OK it looks like universally it just misuses the table, accessing by Z index rather than Z number. I don't know how this has worked before now ... but I do find the British slang appropriately ironic 😉 https://clip.cafe/lock-stock-two-smoking-barrels-1998/i-dont-wanna-know-you-use/

I think the solution is for the muppet code to hard-code Z = 5? I can submit a PR to add an accessor to the table limits.

@amandalund
Copy link
Contributor Author

Haha lord, ok, using the index to set it rather than Z (and access it... sometimes?) is also new since v11.2. Thanks for looking into that more; I'll update the import for the newer versions and I guess just hardcode the size... 🐸 🐷

@sethrj
Copy link
Member

sethrj commented Sep 23, 2024

Oh, so on older versions the table actually is indexed by element (and just has null for the ones that it's interpolating between?) That makes more sense and is a totally different implementation than the current one. See my update that it is actually using just entries 0 through 4; the current version does not expose the {1, 4, 13, 29, 92} element numbers in any way. Guess we will have to switch on version number... or reimplement the cross sections ourselves 🤪

@amandalund
Copy link
Contributor Author

Yeah, the older versions used the table in a more intuitive way/how it was designed to be used.

or reimplement the cross sections ourselves 🤪

I'm tempted at this point ;)

@amandalund
Copy link
Contributor Author

Also it seems to use the Z number to access the muppet in some places, and uses the index to set it. OK it looks like universally it just misuses the table, accessing by Z index rather than Z number.

It is still erroneously accessing the tables by the Z number in G4MuPairProductionModel::StoreTables(), right?

@sethrj
Copy link
Member

sethrj commented Sep 23, 2024

Ah indeed, I got confused because I'd changed some lines but not all. Clearly no one used or tested that method...

@sethrj sethrj merged commit 5cb8d94 into celeritas-project:develop Sep 24, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external Dependencies and framework-oriented features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants