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

refactor: make RNG independent of algorithm order #1905

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Mar 1, 2023

after discussion with paul I though it would be beneficial to remove the dependence of the algorithm order of the RNG in the examples framework. changing the order of reader/writer/algorithms used to result in different outputs which might be unexpected and hard to debug

I believe the impact on the randomness in the simulation is negligible and should not result in funny correlations

@andiwand andiwand added the Component - Examples Affects the Examples module label Mar 1, 2023
@andiwand andiwand added this to the next milestone Mar 1, 2023
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #1905 (6ef3324) into main (3847c73) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1905   +/-   ##
=======================================
  Coverage   49.44%   49.44%           
=======================================
  Files         408      408           
  Lines       22704    22704           
  Branches    10371    10371           
=======================================
  Hits        11226    11226           
  Misses       4266     4266           
  Partials     7212     7212           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@paulgessinger
Copy link
Member

/cc @HadrienG2

Do we think it's a problem if all algorithms get the same random number sequence?

@HadrienG2
Copy link
Contributor

That would result in correlations, which would be directly visible if two copies of the same (or similar) algorithms are run, but I don't think we currently do that often. On different algorithms, I expect the effect to be more subtle.

@github-actions
Copy link

github-actions bot commented Mar 1, 2023

📊 Physics performance monitoring for 6ef3324

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@paulgessinger
Copy link
Member

I think we should probably be ok.
@andiwand do you want to update the changed references?

@andiwand
Copy link
Contributor Author

andiwand commented Mar 1, 2023

will do 👍 at the same time we should then get a 1:1 in your branch if we merge this first

@paulgessinger
Copy link
Member

Docker error?

@andiwand
Copy link
Contributor Author

andiwand commented Mar 1, 2023

something is off with the CI and I just got more conflicts with main 😄

@paulgessinger paulgessinger modified the milestone: next Mar 1, 2023
@paulgessinger
Copy link
Member

Ok I think the CI is back now.

@paulgessinger
Copy link
Member

test_full_chain_odd_example_pythia_geant4 still fails.

@benjaminhuth
Copy link
Member

Just one thought: Would it be useful to make this optional and off by default?
In case someone really wants to run actually two simulations for some reason...

@andiwand
Copy link
Contributor Author

andiwand commented Mar 2, 2023

Just one thought: Would it be useful to make this optional and off by default? In case someone really wants to run actually two simulations for some reason...

I think it would be easier to just pass in two different RNG to the algorithms in this case

@benjaminhuth digging a little bit into the code this seems to be possible. you can give each algorithm a different seed

@andiwand
Copy link
Contributor Author

andiwand commented Mar 2, 2023

test_full_chain_odd_example_pythia_geant4 still fails.

oh no... we have another propagation error with the changed seed

10:21:12    Propagator     ERROR     Propagation reached the step count limit of 10000 (did 10000 steps)
10:21:12    CKF            ERROR     Propapation failed: PropagatorError:3 Propagation reached the configured maximum number of steps with the initial parameters: 
 8.31541
-26.5624
-2.46935
0.751848
-1.74987
  18.233
10:21:12    TrackFinding   WARNING   Track finding failed for seed 10638 with errorPropagatorError:3

@andiwand
Copy link
Contributor Author

andiwand commented Mar 2, 2023

increasing the iteration limit again seems to fix the problem. lets see if this passes this time

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

lgtm

@kodiakhq kodiakhq bot merged commit 8c93c71 into acts-project:main Mar 2, 2023
@github-actions github-actions bot removed the automerge label Mar 2, 2023
@andiwand andiwand deleted the refactor/rng-independent-of-algo-order branch March 2, 2023 12:27
andiwand pushed a commit that referenced this pull request Mar 2, 2023
There's no strong reason to have these as separate objects. This PR will change outputs because of the random number seed generation, see #1905.
@paulgessinger paulgessinger modified the milestones: next, v23.5.0 Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants