Move "inverse of Bz at origin in GeV" from PixelRecoUtilities to MagneticField #35081

makortel · 2021-08-31T01:45:06Z

PR description:

This PR is part of #31061 and attempts to resolve #30560 following the suggestion in #30560 (comment). The "inverse of Bz at origin in GeV" is moved from PixelRecoUtilities as part of MagneticField directly, and the caching of "nominal value in kGauss" is replaced with setting the nominal values in the constructor of the concrete MagneticField class.

Despite of being part of #31061 this PR adds bunch of calls to the deprecated EventSetupRecord::get() function without esConsumes(). This was done to avoid this PR growing too big, because many of the calling classes need to be reworked as well for esConsumes(), and it would be good to get feedback on the approach of this PR early. Those will be addressed in a subsequent PR.

Resolves #30560, resolves cms-sw/framework-team#250

PR validation:

Limited matrix runs. ~~The floating point math should be equivalent to the earlier implementation, and therefore no changes are expected.~~ A tiny regression is observed (numerical changes affecting order of few tracks in 10 events in the PR tests). Further testing showed that 7ca3586 would be enough to restore the previous results.

… nominal values in the constructor. This is a preparatory step to migrate PixelRecoUtilities to esConsumes

…nGeV() In order to migrate the code to esConsumes()

cmsbuild · 2021-08-31T01:52:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35081/24962

This PR adds an extra 136KB to repository

cmsbuild · 2021-08-31T01:53:15Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

FastSimulation/Tracking (fastsim)
FastSimulation/TrajectoryManager (fastsim)
MagneticField/Engine (reconstruction)
MagneticField/ParametrizedEngine (reconstruction)
MagneticField/UniformEngine (reconstruction)
MagneticField/VolumeBasedEngine (reconstruction)
MagneticField/VolumeGeometry (reconstruction)
RecoMuon/TrackerSeedGenerator (reconstruction)
RecoPixelVertexing/PixelLowPtUtilities (reconstruction)
RecoPixelVertexing/PixelTrackFitting (reconstruction)
RecoPixelVertexing/PixelTriplets (reconstruction)
RecoTracker/ConversionSeedGenerators (reconstruction)
RecoTracker/TkHitPairs (reconstruction)
RecoTracker/TkMSParametrization (reconstruction)
RecoTracker/TkSeedGenerator (reconstruction)
RecoTracker/TkTrackingRegions (reconstruction)
RecoVertex/KinematicFitPrimitives (reconstruction)
TrackPropagation/RungeKutta (reconstruction)
TrackingTools/KalmanUpdators (reconstruction)
TrackingTools/PatternTools (reconstruction)
TrackingTools/TrajectoryState (reconstruction)

@civanch, @lveldere, @sbein, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @jpata can you please review it and eventually sign? Thanks.
@felicepantaleo, @abbiendi, @mmusich, @cericeci, @JanFSchulte, @jhgoh, @sscruz, @trocino, @rociovilar, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @ebrondol, @mtosi, @dgulhan, @namapane, @HuguesBrun, @Fedespring, @calderona, @lecriste, @gpetruc, @matt-komm 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

makortel · 2021-08-31T01:58:05Z

@cmsbuild, please test

cmsbuild · 2021-08-31T07:43:25Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4104ab/18152/summary.html
COMMIT: fc60c04
CMSSW: CMSSW_12_1_X_2021-08-30-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35081/18152/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 19 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4104ab/18152/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 184 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000404
DQMHistoTests: Total failures: 4607
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2995775
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-01T18:04:07Z

Limited matrix runs. The floating point math should be equivalent to the earlier implementation, and therefore no changes are expected.

this doesn't quite agree with #35081 (comment) :

Reco comparison results: 184 differences found in the comparisons

DQMHistoTests: Total failures: 4607

slava77 · 2021-09-01T18:06:55Z

@namapane
the updates in MagneticField code in this PR appear to be rather straightforward. Still, please take a look and comment if this looks OK.
Thank you.

makortel · 2021-09-01T18:12:19Z

Limited matrix runs. The floating point math should be equivalent to the earlier implementation, and therefore no changes are expected.

this doesn't quite agree with #35081 (comment) :

Reco comparison results: 184 differences found in the comparisons

DQMHistoTests: Total failures: 4607

Right. I've been investigating those differences. They look like (at most) handful of tracks moving between iterations or having slightly different parameters (i.e. numerical differences), that then cause tiny knock-on effects downstream. I've almost tracked down (in 140.56) which exact change(s) is causing the havoc.

namapane · 2021-09-03T14:27:53Z

@namapane
the updates in MagneticField code in this PR appear to be rather straightforward. Still, please take a look and comment if this looks OK.
Thank you.

I have no major comment apart maybe for the naming of the new method (I find "nominal" to be confusing; "nominalValue()" is named this way because it is intended to be used as a label, while this one is intended to be used for physics, for a very specific use case). Also, using Bz at (0,0,0) instead of the actual value is an approximation that should not be taken as obvious and for this reason I don't particularly like exposing it in the interface... but this is my taste maybe. Anyhow,
a very fast parametrization of Bz is avaliable in the tracker region and would give a much better approximation, without much overhead; I'd suggest considering using it at some point.

slava77 · 2021-09-03T14:32:12Z

I've been investigating those differences. They look like (at most) handful of tracks moving between iterations or having slightly different parameters (i.e. numerical differences), that then cause tiny knock-on effects downstream. I've almost tracked down (in 140.56) which exact change(s) is causing the havoc.

@makortel
Please clarify if the investigation was completed.
Thank you.

slava77 · 2021-09-03T14:38:21Z

I have no major comment apart maybe for the naming of the new method (I find "nominal" to be confusing; "nominalValue()" is named this way because it is intended to be used as a label, while this one is intended to be used for physics, for a very specific use case). Also, using Bz at (0,0,0) instead of the actual value is an approximation that should not be taken as obvious and for this reason I don't particularly like exposing it in the interface... but this is my taste maybe. Anyhow,
a very fast parametrization of Bz is avaliable in the tracker region and would give a much better approximation, without much overhead; I'd suggest considering using it at some point.

@namapane
Thank you for checking the updates.
Do you have possibly an alternative name instead of nominal in this context?
Related to use of parameterization, I seem to recall that even a fast implementation was not justifiable in the context of pixel tracking at least for the phase-1 tracker (extremes extending to at most 16 cm X 50 cm in r X z).

makortel · 2021-09-03T14:44:31Z

Do you have possibly an alternative name instead of nominal in this context?

How about something along inverseInGeVAtZero() or inverseInGeVAtCenter()? (just thinking out loud, most of the times I find figuring out good names hard, and I don't have any ambitions what the name here should be)

slava77 · 2021-09-03T15:14:49Z

How about something along inverseInGeVAtZero() or inverseInGeVAtCenter()? (just thinking out loud, most of the times I find figuring out good names hard, and I have any ambitions what the name here would be)

another option is inverseInGeVAtOrigin ?

makortel · 2021-09-03T15:28:20Z

RecoPixelVertexing/PixelTriplets/src/CAHitQuadrupletGenerator.cc

        for (int i = 0; i < 4; ++i) {
          const GlobalPoint& point = gps[i];
          const GlobalError& error = ges[i];
          bc_r[i] = sqrt(sqr(point.x() - region.origin().x()) + sqr(point.y() - region.origin().y()));
-          bc_r[i] += pixelrecoutilities::LongitudinalBendingCorrection(pt, es)(bc_r[i]);
+          bc_r[i] += pixelrecoutilities::LongitudinalBendingCorrection(pt, *theField)(bc_r[i]);


@slava77

Please clarify if the investigation was completed.

Not quite (I lost yesterday because of CERN's LDAP issues). I have found (in 140.56) that this specific call (in CAHitTripletGenerator as well) somehow makes the difference. That happens even if in PixelRecoUtilities::FieldAt0::FieldAt0() I set the FieldAt0::fieldInInvGev from the MagneticField::inverseNominalValueInGeV() instead of calculating it in the FieldAt0 constructor.

makortel · 2021-09-03T16:06:53Z

How about something along inverseInGeVAtZero() or inverseInGeVAtCenter()? (just thinking out loud, most of the times I find figuring out good names hard, and I have any ambitions what the name here would be)

another option is inverseInGeVAtOrigin ?

Or inverseAtOriginInGeV?

cmsbuild · 2021-09-07T14:36:48Z

Pull request #35081 was updated. @civanch, @lveldere, @sbein, @ssekmen, @mdhildreth, @cmsbuild, @slava77, @jpata can you please check and sign again.

makortel · 2021-09-07T14:39:25Z

@cmsbuild, please test

cmsbuild · 2021-09-07T18:51:09Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4104ab/18361/summary.html
COMMIT: 147924f
CMSSW: CMSSW_12_1_X_2021-09-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35081/18361/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 19 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4104ab/18361/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 230 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3001001
DQMHistoTests: Total failures: 6681
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2994298
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-09T00:16:49Z

+reconstruction

for #35081 147924f

code changes are mostly technical, in line with the PR description and the follow up review
- I agree with the note in the PR description that some es.get is left [or even introduced] to stay practical in the size of this PR
jenkins tests pass and comparisons with the baseline show only small numerical-level differences (expected as discussed earlier and provided in the PR description)

makortel · 2021-09-09T13:04:01Z

@cms-sw/fastsim-l2 Could you review and sign, please?

makortel · 2021-09-10T13:35:03Z

ping @cms-sw/fastsim-l2

civanch · 2021-09-13T10:20:03Z

+1

cmsbuild · 2021-09-13T10:20:27Z

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)

perrotta · 2021-09-13T15:22:56Z

+1

makortel added 2 commits August 31, 2021 03:10

Add "inverse of nominal value" in GeV to MagneticField, and calculate…

5986927

… nominal values in the constructor. This is a preparatory step to migrate PixelRecoUtilities to esConsumes

Migrate PixelRecoUtilities to use MagneticField::inverseNominalValueI…

fc60c04

…nGeV() In order to migrate the code to esConsumes()

cmsbuild added this to the CMSSW_12_1_X milestone Aug 31, 2021

cmsbuild added code-checks-pending fastsim-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 31, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 31, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 31, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 31, 2021

cmsbuild mentioned this pull request Sep 2, 2021

Partial esConsumes migration for TrajectorySeedProducer #35137

Merged

makortel commented Sep 3, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Sep 7, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 7, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 9, 2021

cmsbuild added fastsim-approved fully-signed and removed fastsim-pending pending-signatures labels Sep 13, 2021

cmsbuild added orp-approved and removed orp-pending labels Sep 13, 2021

cmsbuild merged commit 0089507 into cms-sw:master Sep 13, 2021

makortel deleted the esconsumesPixelRecoUtilities branch September 13, 2021 15:26

cmsbuild mentioned this pull request Sep 13, 2021

Configure HCAL FIR Filter Based on HcalTPChannelParameters #35222

Merged

makortel mentioned this pull request Sep 14, 2021

Move MSLayerKeeper* objects from thread-local into EventSetup #35269

Merged

makortel mentioned this pull request Sep 24, 2021

Migrate TrackingRegions to esConsumes #35393

Merged

namapane mentioned this pull request Jul 8, 2022

[12.4.X] Added protection from multiple creation of geometry #38653

Merged

perrotta mentioned this pull request Jul 9, 2022

Make a more robust fix to deal with possible multiple creations of the MF geometries #38669

Closed

This was referenced Jul 12, 2022

SiPixelLorentzAnglePCLHarvester: fetch precise B-field value from map via inverseBzAtOriginInGeV #38700

Merged

[12.4.X] SiPixelLorentzAnglePCLHarvester: fetch precise B-field value from map via inverseBzAtOriginInGeV #38701

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move "inverse of Bz at origin in GeV" from PixelRecoUtilities to MagneticField #35081

Move "inverse of Bz at origin in GeV" from PixelRecoUtilities to MagneticField #35081

makortel commented Aug 31, 2021 •

edited

cmsbuild commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

makortel commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

slava77 commented Sep 1, 2021

slava77 commented Sep 1, 2021

makortel commented Sep 1, 2021

namapane commented Sep 3, 2021

slava77 commented Sep 3, 2021

slava77 commented Sep 3, 2021

makortel commented Sep 3, 2021 •

edited

slava77 commented Sep 3, 2021

makortel Sep 3, 2021

makortel commented Sep 3, 2021

cmsbuild commented Sep 7, 2021

makortel commented Sep 7, 2021

cmsbuild commented Sep 7, 2021

slava77 commented Sep 9, 2021

makortel commented Sep 9, 2021

makortel commented Sep 10, 2021

civanch commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

perrotta commented Sep 13, 2021

Move "inverse of Bz at origin in GeV" from PixelRecoUtilities to MagneticField #35081

Move "inverse of Bz at origin in GeV" from PixelRecoUtilities to MagneticField #35081

Conversation

makortel commented Aug 31, 2021 • edited

PR description:

PR validation:

cmsbuild commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

makortel commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

Comparison Summary

slava77 commented Sep 1, 2021

slava77 commented Sep 1, 2021

makortel commented Sep 1, 2021

namapane commented Sep 3, 2021

slava77 commented Sep 3, 2021

slava77 commented Sep 3, 2021

makortel commented Sep 3, 2021 • edited

slava77 commented Sep 3, 2021

makortel Sep 3, 2021

Choose a reason for hiding this comment

makortel commented Sep 3, 2021

cmsbuild commented Sep 7, 2021

makortel commented Sep 7, 2021

cmsbuild commented Sep 7, 2021

Comparison Summary

slava77 commented Sep 9, 2021

makortel commented Sep 9, 2021

makortel commented Sep 10, 2021

civanch commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

perrotta commented Sep 13, 2021

makortel commented Aug 31, 2021 •

edited

makortel commented Sep 3, 2021 •

edited