eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 27 forks source link

Switch PhotoMultiplierHit RNG to RandomSvc [DO NOT MERGE] #1400

Closed kkauder closed 4 months ago

kkauder commented 4 months ago

Briefly, what does this PR introduce?

Switch PhotoMultiplierHit RNG to RandomSvc

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

Not really, should be faster, lighter, and conform better to reproducibility.

github-actions[bot] commented 4 months ago

Capybara summary for PR 1400

veprbl commented 4 months ago

DRICHRawHits.charge looks systematically offset, but I don't see why.

image

kkauder commented 4 months ago

Could the shift be a statistical artifact? Can you change the seed and try again?

wdconinc commented 4 months ago

Could the shift be a statistical artifact?

The charge distribution shifts up for all test data sets. This is too systematic to be random.

kkauder commented 4 months ago

I undid a few small improvements to check for typos. This version is as minimally altered as possible. If the problem persists, there's a systematic difference between RandomSvc and TMinMax

wdconinc commented 4 months ago

Stop the presses.

bin/eicrecon -Pjana:nevents=100 -Ppodio:output_include_collections=DRICHRawHits -PDRICH:DRICHRawHits:LogLevel=trace sim_dis_18x275_minQ2\=1000_craterlake.edm4hep.root | grep '\[trace\] r = ' | sort | uniq -c

returns

    414 [DRICH:DRICHRawHits] [trace] r = 0.0020400520265848746
    463 [DRICH:DRICHRawHits] [trace] r = 0.006443672428456078
    432 [DRICH:DRICHRawHits] [trace] r = 0.0077978980826384015
    392 [DRICH:DRICHRawHits] [trace] r = 0.01137584773297876
    166 [DRICH:DRICHRawHits] [trace] r = 0.01224292437971925
    208 [DRICH:DRICHRawHits] [trace] r = 0.012752693701702619
    464 [DRICH:DRICHRawHits] [trace] r = 0.01288867816339899
    464 [DRICH:DRICHRawHits] [trace] r = 0.014025674508164564
    336 [DRICH:DRICHRawHits] [trace] r = 0.01427524949300999
    435 [DRICH:DRICHRawHits] [trace] r = 0.01668603467632821
     83 [DRICH:DRICHRawHits] [trace] r = 0.01715572841841742
    464 [DRICH:DRICHRawHits] [trace] r = 0.019521302359817132
    464 [DRICH:DRICHRawHits] [trace] r = 0.020317194038829655
    208 [DRICH:DRICHRawHits] [trace] r = 0.021024228416727027
    218 [DRICH:DRICHRawHits] [trace] r = 0.021358115566709687
    329 [DRICH:DRICHRawHits] [trace] r = 0.02179477655706365
    464 [DRICH:DRICHRawHits] [trace] r = 0.022599992675649817
    448 [DRICH:DRICHRawHits] [trace] r = 0.024394612375319428
    365 [DRICH:DRICHRawHits] [trace] r = 0.03249683305286155
    463 [DRICH:DRICHRawHits] [trace] r = 0.03519841459667793
    428 [DRICH:DRICHRawHits] [trace] r = 0.040132135562983
    464 [DRICH:DRICHRawHits] [trace] r = 0.04318645485609773
    463 [DRICH:DRICHRawHits] [trace] r = 0.04373338491188373
    418 [DRICH:DRICHRawHits] [trace] r = 0.04748356013114665
wdconinc commented 4 months ago

That didn't make sense without this diff:

diff --git a/src/algorithms/digi/PhotoMultiplierHitDigi.cc b/src/algorithms/digi/PhotoMultiplierHitDigi.cc
index 99f16c14..c8d85040 100644
--- a/src/algorithms/digi/PhotoMultiplierHitDigi.cc
+++ b/src/algorithms/digi/PhotoMultiplierHitDigi.cc
@@ -65,7 +65,9 @@ void PhotoMultiplierHitDigi::process(
             trace("hit: pixel id={:#018X}  edep = {} eV", id, edep_eV);

             // overall safety factor
-            if (m_rng.uniform_double<double>(0, 1.0) > m_cfg.safetyFactor) continue;
+            auto r = m_rng.uniform_double<double>(0, 1.0);
+            trace("r = {}", r);
+            if (r > m_cfg.safetyFactor) continue;

             // quantum efficiency
             if (!qe_pass(edep_eV, m_rng.uniform_double<double>(0, 1.0))) continue;
kkauder commented 4 months ago

wtf

wdconinc commented 4 months ago

Closing for now for operational reasons. We can reopen later.