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

removing mkFit from exlude in Eras for PbPb collisions #37939

Merged
merged 1 commit into from Jun 8, 2022

Conversation

milanchestojanovic
Copy link
Contributor

PR description:

This PR enables mkFit on PbPb events. It removes mkFit from "exclude" set in Run2_2018_pp_on_AA and Run3_pp_on_PbPb eras. The change should reduce reconstruction time with similar tracking performances as shown here: https://indico.cern.ch/event/1158382/#3-update-on-mkfit-studies-with.

PR validation:

We performed track reconstruction studies on 2018 PbPb data, and PbPb MC produced for Run3 tests.

@mandrenguyen, @abaty, @CesarBernardes

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37939/29990

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @milanchestojanovic for master.

It involves the following packages:

  • Configuration/Eras (operations)

@cmsbuild, @perrotta, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @missirol, @fabiocos, @Martin-Grunewald this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented May 13, 2022

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-88bc50/24695/summary.html
COMMIT: 6ff6b58
CMSSW: CMSSW_12_4_X_2022-05-12-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37939/24695/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1520 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3697377
  • DQMHistoTests: Total failures: 2794
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3694560
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 206 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented May 17, 2022

@milanchestojanovic
Copy link
Contributor Author

@qliphy
I would say that tracking differences are as expected. I checked other distributions as well and the differences seem small but I am not sure if they are significant.

@mmusich could you please confirm if those differences are within expectations for mkFit?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_4_X_2022-05-12-2300+88bc50/50270/validateJR.html

@mmusich
Copy link
Contributor

mmusich commented May 20, 2022

@milanchestojanovic

could you please confirm if those differences are within expectations for mkFit?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_4_X_2022-05-12-2300+88bc50/50270/validateJR.html

I see that the changes concern only HI reconstruction (wf 140.56 and 312.0) which is expected from the change in the HI-related eras.
About the actual tracking-related changes, I thought you had privately validated them and deemed them acceptable as a trade-off of the reconstruction speed-up, but I am not expert in HI specific tracking and I don't feel comfortable to comment if these are expected or not.
I do see for example that there is in wf 140.56 (2018 Hi data) for the generalTracks collection:

The size of the change seem to be large enough to warrant more detailed investigation. In MC (wf 312.0) the situation is more nuanced.
I am copying also some mkFit expert to cc: @slava77 @mmasciov

@mmusich
Copy link
Contributor

mmusich commented May 20, 2022

assign reconstruction

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

@jpata,@slava77,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented May 20, 2022

a significant reduction of the on-track hit efficiency: https://tinyurl.com/y6khy8xa

BTW, how did the same look like in data validation in mkFit vs no-mkFit?

@mmusich
Copy link
Contributor

mmusich commented May 20, 2022

BTW, how did the same look like in data validation in mkFit vs no-mkFit?

I don't think a RelVal based campaign on data was done

@slava77
Copy link
Contributor

slava77 commented May 20, 2022

BTW, how did the same look like in data validation in mkFit vs no-mkFit?

I don't think a RelVal based campaign on data was done

do the same data-like hit efficiency plots exist in MC? (I can't recall)

@mmusich
Copy link
Contributor

mmusich commented May 20, 2022

do the same data-like hit efficiency plots exist in MC? (I can't recall)

yes, see e.g. https://tinyurl.com/yyg9hbrd (for 312.0). It's run in the "DQM" part of the matrix so by construction it is available both in data and MC.

@clacaputo
Copy link
Contributor

Hi @milanchestojanovic do you have any slides where you performed efficiencies studies with larger statistic samples? Like the usual one provided during mkFit PR, i.e. see description here #37418 (comment)

@mandrenguyen
Copy link
Contributor

We did not consider these kind of quantities. Should I understand that no such behavior was observed when switching from CKF to mkFit in pp? If that's the case, we would have to debug whether this is a feature of mkFit in busy events, or whether it's related to the trajectory filter that was introduced into mkFit to match the heavy ion CKF implementation

Thanks @milanchestojanovic , indeed I can find the same trends in the RECO differences plots; you didn't produce plots like the one pointed out by Marco, like the on-track hit efficiency, right? Or this behaviour is something already studied in HI and accepted as a trade-off?

@clacaputo
Copy link
Contributor

Hi @mandrenguyen , searching a bit in the historical records of mkFit (mkFit people can confirm), I've compared 12_1_0_pre5 where #35660 has been integrated with 12_1_0_pre2 for RelValTTBar_14TeV

The trends seem in line with plots pointed by @mmusich in #37939 (comment)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-88bc50/25039/summary.html
COMMIT: 6ff6b58
CMSSW: CMSSW_12_5_X_2022-05-26-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37939/25039/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1594 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3648322
  • DQMHistoTests: Total failures: 2619
  • DQMHistoTests: Total nulls: 2
  • DQMHistoTests: Total successes: 3645679
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

+reconstruction

@qliphy
Copy link
Contributor

qliphy commented Jun 5, 2022

please test
to refresh

@qliphy
Copy link
Contributor

qliphy commented Jun 7, 2022

please abort
seems to be stuck

@qliphy
Copy link
Contributor

qliphy commented Jun 7, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 7, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-88bc50/25328/summary.html
COMMIT: 6ff6b58
CMSSW: CMSSW_12_5_X_2022-06-06-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37939/25328/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1598 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3651603
  • DQMHistoTests: Total failures: 2629
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3648951
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@mandrenguyen
Copy link
Contributor

@qliphy @perrotta I suppose this one can be included in 12_5_0_pre2, no?

@qliphy
Copy link
Contributor

qliphy commented Jun 8, 2022

+1
differences are understood

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2022

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

@cmsbuild cmsbuild merged commit 1b6e85e into cms-sw:master Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants