[mkFit] Propagate to plane -- technical changes to support further development #43146

osschar · 2023-10-29T10:44:17Z

PR description:

Implement propagation to plane and Kalman update to plane -- work done by @cerati

Propagation to plane is used for propagation to the hit, while propagation to r and z are used for propagation to the layer center. Propagation to the hit starts from the state at the previous hit (by re-calling the mkfndr->inputTracksAndHitIdx(...) ).

This also updates the material effects, as developed by @slava77.

Both changes are currently disabled by default via mkfit::Config switches, making this a technical PR to support further development, especially for layer-processing optimizations & improvements and for supporting Phase2 tilted geometry.

PR validation:

MTV plots in preparation, there should be no significant changes.

cmsbuild · 2023-10-29T10:51:18Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43146/37427

ERROR: Build errors found during clang-tidy run.

RecoTracker/MkFitCore/src/PropagationMPlex.icc:70:3: error: no matching function for call to 'parsFromPathL_impl' [clang-diagnostic-error]
  parsFromPathL_impl(inPar, outPar, kinv, s, nmin, nmax);
  ^~~~~~~~~~~~~~~~~~
RecoTracker/MkFitCore/src/PropagationMPlex.icc:418:3: note: in instantiation of function template specialization 'parsAndErrPropFromPathL_impl<Matriplex::Matriplex<float, 6, 1, 4>, Matriplex::Matriplex<int, 1, 1, 4>, Matriplex::Matriplex<float, 6, 1, 4>, Matriplex::Matriplex<float, 6, 6, 4>, Matriplex::Matriplex<float, 1, 1, 4>>' requested here
--
RecoTracker/MkFitCore/src/PropagationMPlex.icc:383:5: error: no matching function for call to 'parsFromPathL_impl' [clang-diagnostic-error]
    parsFromPathL_impl(inPar, outParTmp, kinv, s, nmin, nmax);
    ^~~~~~~~~~~~~~~~~~
RecoTracker/MkFitCore/src/PropagationMPlex.cc:1094:5: note: in instantiation of function template specialization 'helixAtPlane_impl<Matriplex::Matriplex<float, 6, 1, 4>, Matriplex::Matriplex<int, 1, 1, 4>, Matriplex::Matriplex<float, 6, 1, 4>, Matriplex::Matriplex<float, 3, 1, 4>, Matriplex::Matriplex<float, 6, 6, 4>, Matriplex::Matriplex<float, 1, 1, 4>>' requested here
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild · 2023-10-29T11:14:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43146/37428

This PR adds an extra 176KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoTracker/MkFit/plugins/MkFitGeometryESProducer.cc modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145
- File RecoTracker/MkFitCore/interface/Config.h modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145
- File RecoTracker/MkFitCore/interface/TrackerInfo.h modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145
- File RecoTracker/MkFitCore/src/Matrix.h modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145
- File RecoTracker/MkFitCore/src/MkBuilder.cc modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145
- File RecoTracker/MkFitCore/src/MkFinder.cc modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145
- File RecoTracker/MkFitCore/src/MkFinder.h modified in PR(s): [mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145

cmsbuild · 2023-10-29T11:15:12Z

A new Pull Request was created by @osschar (Matevž Tadel) for master.

It involves the following packages:

RecoTracker/MkFit (reconstruction)
RecoTracker/MkFitCore (reconstruction)

@mandrenguyen, @cmsbuild, @jfernan2 can you please review it and eventually sign? Thanks.
@JanFSchulte, @mtosi, @VourMa, @rovere, @felicepantaleo, @gpetruc, @VinInn, @missirol, @GiacomoSguazzoni, @dgulhan, @makortel, @mmusich this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

slava77 · 2023-10-29T15:32:24Z

@cmsbuild please test

osschar · 2023-10-29T19:44:45Z

There is a minimal conflic between this PR and PR # #43145.
It occurs around MkFitCore/src/MkFinder.cc:170 -- both changes should be accepted.

cmsbuild · 2023-10-29T20:12:19Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e36544/35490/summary.html
COMMIT: 03da373
CMSSW: CMSSW_13_3_X_2023-10-29-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43146/35490/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 37 lines to the logs
Reco comparison results: 18293 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3362691
DQMHistoTests: Total failures: 20917
DQMHistoTests: Total nulls: 3
DQMHistoTests: Total successes: 3341749
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.043 KiB( 49 files compared)
DQMHistoSizes: changed ( 141.044 ): 0.043 KiB JetMET/SUSYDQM
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

mandrenguyen · 2023-11-03T03:43:05Z

RecoTracker/MkFitCore/src/PropagationMPlex.cc

@@ -488,7 +488,19 @@ namespace mkfit {
    errorProp.setVal(0.f);
    outFailFlag.setVal(0.f);

+    //helixAtRFromIterativeCCS_impl_new(inPar, inChg, msRad, outPar, errorProp, outFailFlag, 0, NN, N_proc, pflags);
+    helixAtRFromIterativeCCS_impl(inPar, inChg, msRad, outPar, errorProp, outFailFlag, 0, NN, N_proc, pflags);
+    /*


Is there a good reason to keep all this commented code?

I have removed this (and the much longer implementation in the .icc file) for now and left a comment below with a reference to specific commit in case this needs to be revisited.

antoniovilela · 2023-11-17T22:28:45Z

please test workflow 136.881

cmsbuild · 2023-11-18T04:34:34Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e36544/35934/summary.html
COMMIT: 7fab695
CMSSW: CMSSW_14_0_X_2023-11-17-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43146/35934/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 360 lines to the logs
Reco comparison results: 6916 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3415195
DQMHistoTests: Total failures: 26115
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3389058
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 218 log files, 170 edm output root files, 51 DQM output files
TriggerResults: no differences found

antoniovilela · 2023-11-18T05:54:57Z

+1

antoniovilela · 2023-11-18T05:58:10Z

merge

@cms-sw/reconstruction-l2 signed before squash.

perrotta · 2023-11-18T07:20:53Z

(Unsolicited comment, please ignore if you think so). I understand the urgency to merge this PR. However, given it touches very important areas of tracking I think that the code should be kept cleaner, easier to investigate and worked on, if needed. There are large chunks of commented out code that would at least deserve some comment: in particular, the last rebase only brought in commented out lines of codes. Another thing I noticed: in PropagationMPlex the DEBUG sections may not work, because the definition of the debug variable (at least) is also commented out. Did you ever try it? I suspect that some extra gymnastic would be needed to allow debugging, besides just switching the DEBUG flag at line

cmssw/RecoTracker/MkFitCore/src/PropagationMPlex.cc

Line 8 in 7fab695

//#define DEBUG

antoniovilela · 2023-11-18T11:16:00Z

(Unsolicited comment, please ignore if you think so). I understand the urgency to merge this PR. However, given it touches very important areas of tracking I think that the code should be kept cleaner, easier to investigate and worked on, if needed. There are large chunks of commented out code that would at least deserve some comment: in particular, the last rebase only brought in commented out lines of codes. Another thing I noticed: in PropagationMPlex the DEBUG sections may not work, because the definition of the debug variable (at least) is also commented out. Did you ever try it? I suspect that some extra gymnastic would be needed to allow debugging, besides just switching the DEBUG flag at line

cmssw/RecoTracker/MkFitCore/src/PropagationMPlex.cc

Line 8 in 7fab695

//#define DEBUG

Thanks Andrea.
@osschar @slava77
Matevž, could you follow up?

osschar · 2023-11-18T21:12:51Z

Indeed, the last commit from before the rebase, c9b4d5e, has been lost. I must have made a mistake, I apologize for this. How do you want me to handle this, open a new PR with just this change?

The DEBUG thing is like this by design -- it allows us to switch debug printouts on/off with finer granularity than on the level of whole files.
https://github.com/cms-sw/cmssw/blob/master/RecoTracker/MkFitCore/src/Debug.h

antoniovilela · 2023-11-18T22:45:05Z

Indeed, the last commit from before the rebase, c9b4d5e, has been lost. I must have made a mistake, I apologize for this. How do you want me to handle this, open a new PR with just this change?

The DEBUG thing is like this by design -- it allows us to switch debug printouts on/off with finer granularity than on the level of whole files. https://github.com/cms-sw/cmssw/blob/master/RecoTracker/MkFitCore/src/Debug.h

Hi Matevž,
Yes, you can open a new PR. In the description mention it is a follow up of this.
To squash the commits into one, you could use the git cms-squash-topic command, introduced recently (https://cms-sw.github.io/tutorial-resolve-conflicts.html).
Thanks.

jfernan2 · 2023-11-20T09:53:07Z

+1

cmsbuild · 2023-11-20T09:53:32Z

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.

osschar · 2023-11-22T20:47:49Z

Thank you @antoniovilela! I have opened a follow up PR, #43356.

antoniovilela · 2023-11-23T21:29:02Z

Thank you @antoniovilela! I have opened a follow up PR, #43356.

Thanks

[MkFit] Remove commented-out code, follow up to #43146

cmsbuild added this to the CMSSW_13_3_X milestone Oct 29, 2023

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending tracking labels Oct 29, 2023

cmsbuild mentioned this pull request Oct 29, 2023

[mkFit] Tools supporting better layer-related computations -- technical changes to support further development #43145

Merged

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Oct 29, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 29, 2023

cmsbuild added tests-started and removed tests-pending labels Oct 29, 2023

cmsbuild added tests-approved and removed tests-started labels Oct 29, 2023

osschar mentioned this pull request Oct 30, 2023

Reinactment of 135 CMMSW PR -- Prop to plane pr trackreco/cmssw#136

Merged

mandrenguyen reviewed Nov 3, 2023

View reviewed changes

cmsbuild added tests-pending code-checks-pending and removed tests-approved code-checks-approved labels Nov 3, 2023

cmsbuild added the tests-rejected label Nov 17, 2023

cmsbuild added tests-started and removed tests-rejected labels Nov 17, 2023

cmsbuild added tests-approved and removed tests-started labels Nov 18, 2023

cmsbuild added orp-approved and removed orp-pending labels Nov 18, 2023

cmsbuild merged commit 9e9d289 into cms-sw:master Nov 18, 2023
11 checks passed

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Nov 20, 2023

leonardogiannini mentioned this pull request Nov 21, 2023

[mkFit] Apply material effects after error propagation #43350

Merged

This was referenced Nov 22, 2023

[MkFit] Remove commented-out code, follow up to #43146 trackreco/cmssw#140

Merged

[MkFit] Remove commented-out code, follow up to #43146 #43356

Merged

cmsbuild added a commit that referenced this pull request Nov 23, 2023

Merge pull request #43356 from trackreco/prop-to-plane-fix-pr

05b777b

[MkFit] Remove commented-out code, follow up to #43146

slava77 mentioned this pull request Nov 24, 2023

[NONLTO_X] "dangling-pointer" warnings in mkfitcore #43380

Closed

dan131riley mentioned this pull request Feb 7, 2024

[CLANG_X] Loop not vectorized in MkFit #43886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mkFit] Propagate to plane -- technical changes to support further development #43146

[mkFit] Propagate to plane -- technical changes to support further development #43146

osschar commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

slava77 commented Oct 29, 2023

osschar commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

mandrenguyen Nov 3, 2023

osschar Nov 12, 2023

antoniovilela commented Nov 17, 2023

cmsbuild commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

perrotta commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

osschar commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

jfernan2 commented Nov 20, 2023

cmsbuild commented Nov 20, 2023

osschar commented Nov 22, 2023

antoniovilela commented Nov 23, 2023

[mkFit] Propagate to plane -- technical changes to support further development #43146

[mkFit] Propagate to plane -- technical changes to support further development #43146

Conversation

osschar commented Oct 29, 2023

PR description:

PR validation:

cmsbuild commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

slava77 commented Oct 29, 2023

osschar commented Oct 29, 2023

cmsbuild commented Oct 29, 2023

Comparison Summary

mandrenguyen Nov 3, 2023

Choose a reason for hiding this comment

osschar Nov 12, 2023

Choose a reason for hiding this comment

antoniovilela commented Nov 17, 2023

cmsbuild commented Nov 18, 2023

Comparison Summary

antoniovilela commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

perrotta commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

osschar commented Nov 18, 2023

antoniovilela commented Nov 18, 2023

jfernan2 commented Nov 20, 2023

cmsbuild commented Nov 20, 2023

osschar commented Nov 22, 2023

antoniovilela commented Nov 23, 2023