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

Implement step limiter (G4UserLimits) #130

Merged
merged 230 commits into from
Jun 20, 2023
Merged

Conversation

nkrah
Copy link
Collaborator

@nkrah nkrah commented Mar 3, 2023

This PR bundles a series of commits to implement step size limitation per volume (region) in opengate, plus some bug fixes and cosmetic improvements along the way.

It required new binding for some Geant4 classes and a thin wrapper around the G4RunManager to expose essential protected members. Changes mainly appear in Simulation and SimulationEngine classes, in VolumeManager and VolumeEngine, as well as PhysicsManager and PhysicsEngine classes.

Note 1: The initialization order in SimulationEngine.initialize was changed because initializing the step limiter requires an initialized geometry (G4RunManager.InitializeGeometry). Only then can G4RunManager.InitializePhysics be called. This has to do with the states handled by the G4StateManager. The G4RunManager.Initialize function would internally initialize geometry and physics in one go, which is incompatible with step limiter initialization.
Please refer to G4RunManager.cpp and exampleB2b in the Geant4 source.

Note 2: The step limiter is only handled correctly in single thread mode. Multithread mode would also require a wrapped RunManager (see note 1). As a temporary workaround, a check might need to be added to raise an error if a step limiter is requested in MT mode.

* Include Getters/Setters for UserLimits
* Change return policies for Getters
* set py::nodelete for G4Region to prevent python's garbage collection
…tput from the hook function such as SimulationEngine.user_fct_after_init()
@nkrah
Copy link
Collaborator Author

nkrah commented Mar 3, 2023

I just saw that PR #123 also mentions UserLimits. It's probably worth cross-checking.

@nkrah nkrah mentioned this pull request Mar 6, 2023
@nkrah
Copy link
Collaborator Author

nkrah commented Mar 7, 2023

Do not merge yet. I will still extend and modify this PR.

@tbaudier
Copy link
Contributor

tbaudier commented Mar 7, 2023

@nkrah
Copy link
Collaborator Author

nkrah commented Jun 18, 2023

Update:
tests in CI timed out. I downloaded the wheel, installed it locally in a proper environment. Tests got stuck at test023_filters_iec_phantom.py at the end of the simulation.
Strange thing: the simulation runs fine within the src directory of my local code, i.e. which uses directly my local python code, not the package installed from the wheel.

I will do a full local run of tests over night and see what comes out.

@nkrah
Copy link
Collaborator Author

nkrah commented Jun 18, 2023

Another update:
CI just now failed because of tissue after updated colored package (see issue #176 ).

@nkrah
Copy link
Collaborator Author

nkrah commented Jun 18, 2023

Provided a fix for broken syntax after updating colored package to v1.5

@nkrah
Copy link
Collaborator Author

nkrah commented Jun 19, 2023

Implemented fixes in parallel world handling.

They were due to that part being implemented in parallel to structural changes in this PR.

@nkrah
Copy link
Collaborator Author

nkrah commented Jun 19, 2023

Locally, i.e. on MacOS, python 3.9, all tests pass.

@nkrah
Copy link
Collaborator Author

nkrah commented Jun 19, 2023

In test045 (GANs), I had incorrectly copied the cut settings from the old version. Fixed now.

test013_phys_list_1.py was slightly out of stat tolerance. Increased tolerance from 10% to 12%.
Note: Also with the current master, I get -10% tracks compared to ref stats and about 59% fewer gammas. I get the same difference between my PR and the ref. So I am quite certain this is not due to changes introduced by the PR.
All the verbose output from Geant4 looks reasonable for this test.

@dsarrut dsarrut merged commit e6d075b into OpenGATE:master Jun 20, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants