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

[12.1.X] Update of mkFit as in 12_2_0_pre3 (backport of PR #36246) #36315

Merged
merged 1 commit into from Dec 3, 2021

Conversation

mmasciov
Copy link
Contributor

@mmasciov mmasciov commented Dec 1, 2021

PR description:

Backport of PR #36246 to 12_1_X, together with cms-sw/cmsdist#7482.
The reason for the backport is to ensure that data reprocessing makes use of the same mkFit developments available in 12_2_X.

It should be integrated together with cms-sw/cmsdist#7482.

PR validation:

Please refer to PR #36246.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

A new Pull Request was created by @mmasciov (Mario Masciovecchio) for CMSSW_12_1_X.

It involves the following packages:

  • RecoTracker/MkFit (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @mmusich, @mtosi, @dgulhan 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

@mmasciov mmasciov changed the title [12.1.X] Update of mkFit as in 12_2_0_pre3 (backport of PR #35246) [12.1.X] Update of mkFit as in 12_2_0_pre3 (backport of PR #36246) Dec 1, 2021
@slava77
Copy link
Contributor

slava77 commented Dec 1, 2021

test parameters:

@slava77
Copy link
Contributor

slava77 commented Dec 1, 2021

@cmsbuild please test

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

backport of #36246

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

urgent

  • this is needed for the ReRECO

@jpata
Copy link
Contributor

jpata commented Dec 1, 2021

Just to make explicit: we expect to ignore the no-change rule here, correct?

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

Just to make explicit: we expect to ignore the no-change rule here, correct?

I confirmed with PdmV that there weren't any samples created with CMSSW_12_1_0 yet (also no data yet) so in the end there is no conflict here, we should just keep in mind not to do samples with CMSSW_12_1_0 but only with the new CMSSW_12_1_1 (or whatever higher)

@JanFSchulte
Copy link
Contributor

Do we need a similar backport for the updated DNN in #36285?

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

Do we need a similar backport for the updated DNN in #36285?

@mmusich would that affect the alignment?

@mmusich
Copy link
Contributor

mmusich commented Dec 1, 2021

@mmusich would that affect the alignment?

Ideally I would like to test something as close as the Run3 production environment, but at some point we need to cut the actual release.
I would stll be keen to understand what's the difference between the scenario in which #36285 is backported and the one in which is not. Perhaps @mmasciov can help here.

@mmasciov
Copy link
Contributor Author

mmasciov commented Dec 1, 2021

@mmusich would that affect the alignment?

Ideally I would like to test something as close as the Run3 production environment, but at some point we need to cut the actual release. I would stll be keen to understand what's the difference between the scenario in which #36285 is backported and the one in which is not. Perhaps @mmasciov can help here.

The difference can be seen from cms-data/RecoTracker-FinalTrackSelectors#11 (comment).
For high-purity tracks:
https://mmasciovecchio.web.cern.ch/BTV_Nov2021/MTV_TTbarPU50_mkFit_for1220pre3_DNNWP/plots_highPurity/effandfakePtEtaPhi.pdf
By comparing orange to red curves, one can see that PR #36285 allows to reduce fake tracks, while retaining the same track reconstruction efficiency.
The same holds for out-of-the-box tracks with loose (not high-purity) quality requirement (https://mmasciovecchio.web.cern.ch/BTV_Nov2021/MTV_TTbarPU50_mkFit_for1220pre3_DNNWP/plots_ootb/effandfakePtEtaPhi.pdf).
To summarize, IMHO a backport of #36285 would be good. Without it, the amount of fake tracks would be different in 121X wrt. 122X.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5096b0/20902/summary.html
COMMIT: 6fe7825
CMSSW: CMSSW_12_1_X_2021-12-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36315/20902/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: 18273 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 2901440
  • DQMHistoTests: Total failures: 25139
  • DQMHistoTests: Total nulls: 6
  • DQMHistoTests: Total successes: 2876273
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.078 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 136.793 ): 0.066 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 136.874 ): 0.012 KiB JetMET/SUSYDQM
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: found differences in 1 / 41 workflows

@JanFSchulte
Copy link
Contributor

I have created a PR for the DNN backport: #36327.

@jpata
Copy link
Contributor

jpata commented Dec 2, 2021

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_1_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_2_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Dec 3, 2021

+1

@cmsbuild cmsbuild merged commit df2f9a0 into cms-sw:CMSSW_12_1_X Dec 3, 2021
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