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

add quality selections in MkFitOutputConverter #37139

Merged

Conversation

slava77
Copy link
Contributor

@slava77 slava77 commented Mar 4, 2022

This PR intends to address #35798, to reduce warnings about bad track states downstream of mkFit track building.

In addition to already present check for positive-definiteness of the covariance based on the diagonal elements, more selections were added to MkFitOutputConverter in order to send only trackable candidates:

  • the covariance matrix is checked using the Sylvester's criterion to have a full check of positive-definiteness
  • "sanity" checks for the track candidate parameters:
    • pt > 10 MeV
    • θ ∈ [0.01, π–0.01]
    • state position inside the tracker: r < 120 cm and |z| < 280 cm; position uncertainty below 100 cm

The CPU cost in the converter goes up by about 0.02% of the reco time based on wf 136.874: before vs after

The number of logWarnings went down, focusing on the ones with not pos in the warning message (using CMSSW_12_3_0_pre1 as a reference):

  • in 136.874 on 100 events: baseline 112, noMkFit baseline 3, this PR 12.
    • the warnings are in modules not directly reading mkFit track candidates
  • in wf 11834.21 on 400 events: baseline 2636, noMkFit 31, this PR 19
    • the warnings are in modules not directly reading mkFit track candidates
  • in a trackingOnly test similar to 11834.21 with PU50 on 1000 events: baseline 2209, noMkFit 19, this PR 17
    • here 14 warnings are in modules reading mkFit track candidates

On average, it seems that the number of warnings is roughly back to the rate seen with the old setup using just the CKF builder.

Small changes are expected due to rejection of mainly bad tracks.
@leonardogiannini checked in a sample with pileup 50 on 1000 evens, there are negligible or no differences in the MTV plots:
looking at all tracks
image
The effect is a bit more visible in the pixelLess iteration, the most affected iteration
image

@osschar @makortel @mmasciov
@cms-sw/tracking-pog-l2

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37139/28676

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2022

A new Pull Request was created by @slava77 (Slava Krutelyov) for master.

It involves the following packages:

  • RecoTracker/MkFit (reconstruction)

@jpata, @cmsbuild, @clacaputo, @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

@slava77
Copy link
Contributor Author

slava77 commented Mar 4, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5bbd24/22842/summary.html
COMMIT: 4af1441
CMSSW: CMSSW_12_3_X_2022-03-04-0800/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37139/22842/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: 2338 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3987741
  • DQMHistoTests: Total failures: 9233
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3978486
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@slava77
Copy link
Contributor Author

slava77 commented Mar 4, 2022

The effect is a bit more visible in the pixelLess iteration, the most affected iteration
image

I've added the above observation to the PR description, since most of the logWarnings were in the pixelLess iteration.

@slava77
Copy link
Contributor Author

slava77 commented Mar 7, 2022

@cms-sw/reconstruction-l2
it would be nice to get this in pre6

@perrotta
Copy link
Contributor

perrotta commented Mar 7, 2022

urgent

@cms-sw/reconstruction-l2 it would be nice to get this in pre6

@cmsbuild cmsbuild added the urgent label Mar 7, 2022
@jpata
Copy link
Contributor

jpata commented Mar 7, 2022

+reconstruction

  • minor changes to low-pt tracks
  • reduces warning verbosity

@jpata
Copy link
Contributor

jpata commented Mar 7, 2022

assign @cms-sw/tracking-pog-l2

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 7, 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 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)

@mmusich
Copy link
Contributor

mmusich commented Mar 7, 2022

assign https://github.com/orgs/cms-sw/teams/tracking-pog-l2

not sure if you really wanted to assign this to trk-pog.
In any case @slava77 will you be able to summarize all the activity done towards the mitigation of #35798 at the next TRK POG meeting on March 14th?

@slava77
Copy link
Contributor Author

slava77 commented Mar 7, 2022

In any case @slava77 will you be able to summarize all the activity done towards the mitigation of #35798 at the next TRK POG meeting on March 14th?

sure; towards the end of the agenda, if possible.
Also, this may become a part of a larger mkFit update, but I'm not sure yet if that part converges by March 14.

@perrotta
Copy link
Contributor

perrotta commented Mar 7, 2022

assign @cms-sw/tracking-pog-l2

@jpata could you please clarify if you meant that this PR can be merged even now as it is, or if you suggest to wait for some kind of additional blessing by the tracking POG instead? Would that blessing arrive in time for the pre6 deadilne, if so?

@mmusich
Copy link
Contributor

mmusich commented Mar 7, 2022

Also, this may become a part of a larger mkFit update, but I'm not sure yet if that part converges by March 14.

Thanks @slava77 here's the slot: https://indico.cern.ch/event/1133724/#4-mkfit-status-report-quality

Would that blessing arrive in time for the pre6 deadilne,

As far as I am concerned the blessing can be done immediately, based on the report above: #37139 (comment)

@perrotta
Copy link
Contributor

perrotta commented Mar 7, 2022

+1

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

6 participants