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

Add high-level user/fast sim integration helpers #1615

Merged
merged 21 commits into from
Feb 13, 2025

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Feb 7, 2025

This completes the first bullet on #1600 . It deprecates SimpleOffload and adds new integration classes for UserAction and FastSimulation . I'm using inheritance for implementation rather than interface, but it substantially cuts down the boilerplate at the expense of making the class harder to read (you need to open two classes to get the full interface).

@sethrj sethrj added enhancement New feature or request external Dependencies and framework-oriented features labels Feb 7, 2025
Copy link

github-actions bot commented Feb 7, 2025

Test summary

 4 592 files   7 030 suites   13m 24s ⏱️
 1 710 tests  1 704 ✅  6 💤 0 ❌
24 441 runs  24 357 ✅ 84 💤 0 ❌

Results for commit 448084f.

♻️ This comment has been updated with latest results.

@whokion
Copy link
Contributor

whokion commented Feb 11, 2025

@sethrj A quick test with fastsim-offload of this branch (built with Geant4 11.3) has an exception with

info: Activating Celeritas version 0.5.0-153+f2c0e17d on CPU

-------- EEEE ------- G4Exception-START -------- EEEE -------
*** G4Exception : celer.init.global
      issued by : accel/SharedParams.cc:471
Celeritas runtime error: along-step action factory 'make_along_step' was not defined in the celeritas::SetupOptions
*** Fatal Exception *** core dump ***
 **** Track information is not available at this moment
 **** Step information is not available at this moment

-------- EEEE -------- G4Exception-END --------- EEEE -------

even though it seems that make_along_step is set at this line.
I also had to change the version of Celeritas from 0.6 to 0.5,
find_package(Celeritas 0.6 REQUIRED)
in CMakeLists.txt (as it has not been tagged to v.06 yet, I guess). Other two examples under accel run fine. Anyway, excellent works to support all uses cases!

@sethrj
Copy link
Member Author

sethrj commented Feb 11, 2025

Thanks @whokion , I'll see if I can reproduce that odd failure 🤔

Regarding the version, perhaps you need to run git fetch upstream --tags?

@whokion
Copy link
Contributor

whokion commented Feb 12, 2025

Okay, the version problem is resolved by git fetch upstream --tags. Regarding to the crash, it seems that MakeOptions() is never set to FastSimulationIntegration::Instance().SetOptions(MakeOptions()) anywhere (?).

@sethrj
Copy link
Member Author

sethrj commented Feb 12, 2025

Great catch @whokion , I had accidentally disabled execution of the fastsim example in our CI. I'm adding additional checking to ensure that the options are set before beginning.

Copy link
Contributor

@amandalund amandalund left a comment

Choose a reason for hiding this comment

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

Looks like fastsim might be trying to flush without Celeritas enabled? Looks great on my end, thanks @sethrj!

@whokion
Copy link
Contributor

whokion commented Feb 12, 2025

I'm adding additional checking to ensure that the options are set before beginning.

Thanks for quick updating, which fixes the crash!

@whokion
Copy link
Contributor

whokion commented Feb 12, 2025

@sethrj One more minor comment on TrackingManagerConstructor: this class name is confusing as it sounds like constructing a TrackingManager rather than a PhysicsConstructor. A concrete class of G4VPhysicsConstructor is meant to be added to a modular physics list, so its name may be better to contain "Physics" (this is also a common practice in all Geant4 physics constructors). So, suggested name may be OffloadEmPhysics or something similar. Also, the OffloadParticles still be included in ConstructParticle() rather than empty even though they are overloaded somewhere (where?) without any side effect, which clarifies what particles are specifically involved for this physics constructor. On the other hand, this is still purely a semantic argument.

@whokion
Copy link
Contributor

whokion commented Feb 12, 2025

One more minor comment (sorry that I have not followed this MR from the beginning, so do not see all updated changes in "Files Changed"): In the main of three examples under example/acc, the default particle of PrimaryGeneratorAction is set to 100 MeV neutron, which will not produce any meaningful EM or hadronic secondaries as 100 MeV is below the pion production threshold, and the neutron mainly undergoes elastic scatterings and only produces very soft gammas during the deexcitation process even if there is (rare) an inelastic inter-nuclear interaction or a neutron capture process. So, it would be better to set to something that can generate secondary hadrons and then produce EM particles from either inelastic or the ${\pi}_o$ production. Anyway, 1 GeV (or higher energy) pion-/+ may be a good default.

@sethrj sethrj disabled auto-merge February 13, 2025 00:49
@sethrj
Copy link
Member Author

sethrj commented Feb 13, 2025

@whokion Good point. I dialed down the energy to reduce the compute time since it's just a silly example, but maybe I'll just make the world smaller to let some of the energy escape.

@sethrj sethrj merged commit 6580368 into celeritas-project:develop Feb 13, 2025
38 checks passed
@sethrj sethrj deleted the user-action-integration branch February 13, 2025 13:59
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.

3 participants