cms-patatrack / cmssw

CMSSW fork of the Patatrack project
https://patatrack.web.cern.ch/patatrack/index.html
Apache License 2.0
2 stars 5 forks source link

Clean up the pixel track reconstruction code #606

Closed ericcano closed 3 years ago

ericcano commented 3 years ago

Updated EDM access:

Style fixes:

Avoided some code repetitions.

fwyzard commented 3 years ago

Validation summary

Reference release CMSSW_11_3_0_pre3 at 3220826ed091 Development branch cms-patatrack/CMSSW_11_3_X_Patatrack at 0972c2f697d2 Testing branch cms-patatrack/CMSSW_11_3_X_Patatrack at 0972c2f697d2 with PRs:

Validation plots

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Validation plots (CPU vs GPU)

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Throughput plots

/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53

logs and nvprof/nvvp profiles

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Logs

The full log is available at https://patatrack.web.cern.ch/patatrack/validation/pulls/54e450da99f3748380b353d2bc5bd5377634fd91/log .

fwyzard commented 3 years ago

Validation summary

Reference release CMSSW_11_3_0_pre3 at 3220826ed091 Development branch cms-patatrack/CMSSW_11_3_X_Patatrack at 0972c2f697d2 Testing branch cms-patatrack/CMSSW_11_3_X_Patatrack at 0972c2f697d2 with PRs:

Validation plots

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Validation plots (CPU vs GPU)

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Throughput plots

/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53

scan-136.885502.png zoom-136.885502.png scan-136.885512.png zoom-136.885512.png scan-136.885522.png zoom-136.885522.png

logs and nvprof/nvvp profiles

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Logs

The full log is available at https://patatrack.web.cern.ch/patatrack/validation/pulls/dc4a33b2b9e027ca3e74a9414b31ab501444924b/log .

fwyzard commented 3 years ago

Validation summary

Reference release CMSSW_11_3_0_pre5 at 23fad57b764b Development branch cms-patatrack/CMSSW_11_3_X_Patatrack at f619c431302a Testing branch cms-patatrack/CMSSW_11_3_X_Patatrack at f619c431302a with PRs:

Validation plots

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Validation plots (CPU vs GPU)

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Throughput plots

/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53

scan-136.885502.png zoom-136.885502.png scan-136.885512.png zoom-136.885512.png scan-136.885522.png zoom-136.885522.png

logs and nvprof/nvvp profiles

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Logs

The full log is available at https://patatrack.web.cern.ch/patatrack/validation/pulls/ed18a8dc6db957143d019459de96d9e4345cfb4c/log .

fwyzard commented 3 years ago

@ericcano Commented out vertexing steps in config customization.

Don't do that, please. This PR will be merged into the Patatrack branch, and needs to have all functionality in place.

This kind of changes should be done only in the PR for upstream CMSSW.

ericcano commented 3 years ago

@ericcano Commented out vertexing steps in config customization.

Don't do that, please. This PR will be merged into the Patatrack branch, and needs to have all functionality in place.

This kind of changes should be done only in the PR for upstream CMSSW.

OK. Pushed removed (-f) the commit from the branch.

fwyzard commented 3 years ago

Do you think the branch is in a good shape to be tested, merged in Patatrack, and for making the upstream PR ?

ericcano commented 3 years ago

Do you think the branch is in a good shape to be tested, merged in Patatrack, and for making the upstream PR ?

I just found the bug in the simplification/refactoring of work division (* instead of +). I will quickly test it and push.

Besides, the only thing left is remarks about pixHits.ipynb cleanup in: https://github.com/cms-sw/cmssw/pull/31722#pullrequestreview-560047687

fwyzard commented 3 years ago

Validation summary

Reference release CMSSW_11_3_0_pre5 at 23fad57b764b Development branch cms-patatrack/CMSSW_11_3_X_Patatrack at f619c431302a Testing branch cms-patatrack/CMSSW_11_3_X_Patatrack at f619c431302a with PRs:

Validation plots

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Validation plots (CPU vs GPU)

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Throughput plots

/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53

scan-136.885502.png zoom-136.885502.png scan-136.885512.png zoom-136.885512.png scan-136.885522.png zoom-136.885522.png

logs and nvprof/nvvp profiles

/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW

/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW

Logs

The full log is available at https://patatrack.web.cern.ch/patatrack/validation/pulls/3f53ecdcf234509ddf078f779ca533b121dc9111/log .

fwyzard commented 3 years ago

The failures in the .511 and .512 workflows are due to using an older global tag, which is lacking the correct payloads for the ECAL reconstruction. Since this PR does not touch it, they can be ignored for the time being.

fwyzard commented 3 years ago

The physics results are unchanged, as expected.

fwyzard commented 3 years ago

The throughput seems to have a minor regression with 10-12 threads, which is unexpected but also within the uncertainty of the measurement.

fwyzard commented 3 years ago

We can merge the PR as is, to move forward with the integration work, but @ericcano could you run a manual comparison of the throughput before/after this PR, to confirm that the changes are not significant ?

fwyzard commented 3 years ago

could you run a manual comparison of the throughput before/after this PR, to confirm that the changes are not significant ?

in fact I realised only now that these throughput results are not reliable: they are done with a single job, which has a maximum I/O throughput around 1100 ev/s - so the throughput of the job that does the event processing is so close to that to be useless.

Re-running by hand with 2 jobs each with 10 threads, I get:

So there is no impact on the throughput as well (as expected).