refactor: correct phi and theta for periodicity changes in KalmanVertexUpdater

[felix-russo]

will break athena

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b0432e5) 48.90% compared to head (93b3f13) 48.90%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2769   +/-   ##
=======================================
  Coverage   48.90%   48.90%           
=======================================
  Files         494      494           
  Lines       28822    28822           
  Branches    13668    13668           
=======================================
  Hits        14095    14095           
  Misses       4874     4874           
  Partials     9853     9853           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

📊: Physics performance monitoring for 93b3f13

Full contents

physmon summary

• ✅ CKF truth_smeared
• ✅ IVF truth_smeared
• ✅ AMVF truth_smeared
• ✅ Track Summary CKF truth_smeared
• ✅ Seeding truth_estimated
• ✅ CKF truth_estimated
• ✅ IVF truth_estimated
• ✅ AMVF truth_estimated
• ✅ Track Summary CKF truth_estimated
• ✅ Seeding seeded
• ✅ CKF seeded
• ✅ IVF seeded
• ✅ AMVF seeded
• ✅ AMVF (+grid seeder) seeded
• ✅ Track Summary CKF seeded
• ✅ Seeding orthogonal
• ✅ CKF orthogonal
• ✅ IVF orthogonal
• ✅ AMVF orthogonal
• ✅ Track Summary CKF orthogonal
• ✅ Ambisolver seeded
• ✅ Ambisolver orthogonal
• ✅ Seeding ttbar
• ✅ CKF ttbar
• ✅ Ambisolver
• ✅ Track Summary CKF ttbar
• ✅ AMVF ttbar
• ✅ AMVF (+grid seeder) ttbar
• ✅ Truth tracking (GSF)
• ✅ Truth tracking
• ✅ Particles fatras
• ✅ Particles geant4

[paulgessinger]

So we can end up with phi-theta values that are outside $[0,\pi)$ and $[-\pi,\pi]$ and this just wraps it back to the valid ranges?

Seems like a no-brainer to me.

I triggered the Athena jobs, let's see what this looks like now.

[acts-project-service]

✅ Athena integration test results [d095879]

✅ All tests successful

status	job	report
🟢	run_unit_tests
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsEFTrackFit.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsBenchmarkWithSpot.sh 8 100
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsWorkflow.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateAmbiguityResolution.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateResolvedTracks.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateTracks.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateActsCoreSpacePoints.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateActsSpacePoints.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateSeeds.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateOrthogonalSeeds.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateClusters.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsPersistifyEDM.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsGSFRefitting.sh
🟢	run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsKfRefitting.sh
🟢	run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsExtrapolationAlgTest.py
🟢	run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsITkTest.py
🟢	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🟢	run_workflow_tests_run3_data
🟢	run_art_test: test_data18_13TeV_1000evt
🟢	run_art_test: test_ttbarPU40_reco

[paulgessinger]

So in the run 2 test it changes the vertex chi2 on 2 vertices in the ESD and 3 vertices in the AOD and in the run 3 test it's 2 vertices changed in the AOD and 10 vertices in the ESD.

You can see this in the data18 ART job here:

[felix-russo]

Yes that's why we kept this change for the next major version!

As usual, I don't know if this makes athena "worse" or "better". But I think we have enough arguments to get this in

[paulgessinger]

@felix-russo I guess this we'll have to argue.

[asalzburger]

Hi, I think - as the phi wrapping into range makes totally sense, we should argue for this change.

[andiwand]

@paulgessinger and I had a discussion about this last week and we think if we wrap the angle we would also have to modify the chi2 code

@paulgessinger did you check the results after wrapping the difference again?

[felix-russo]

FYI: this is also done at a different place of the Kalman Updater; I think it was just forgotten here:

acts/Core/src/Vertexing/KalmanVertexTrackUpdater.cpp

Lines 148 to 153 in 2dbf768

	// Get phi and theta and correct for possible periodicity changes
	const auto correctedPhiTheta =
	Acts::detail::normalizePhiTheta(newTrkMomentum(0), newTrkMomentum(1));
	newTrkParams(BoundIndices::eBoundPhi) = correctedPhiTheta.first; // phi
	newTrkParams(BoundIndices::eBoundTheta) = correctedPhiTheta.second; // theta
	newTrkParams(BoundIndices::eBoundQOverP) = newTrkMomentum(2); // qOverP

[andiwand]

the problem I see is that we later subtract other track params which are not pi/2pi wrapped

acts/Core/src/Vertexing/KalmanVertexUpdater.cpp

Lines 121 to 135 in e283c46

	// Correct phi and theta for possible periodicity changes
	const auto correctedPhiTheta =
	Acts::detail::normalizePhiTheta(newTrkMom(0), newTrkMom(1));
	newTrkMom(0) = correctedPhiTheta.first; // phi
	newTrkMom(1) = correctedPhiTheta.second; // theta
	// \tilde{p_k}
	ParameterVector linearizedTrackParameters =
	constTerm + posJacVtxPos + momJac * newTrkMom;
	// r_k
	ParameterVector paramDiff = trkParams - linearizedTrackParameters;
	// Return chi2
	return paramDiff.transpose() * (trkParamWeight * paramDiff);

so not correcting them in the first place will result in small differences while correcting the angle needs another difference wrapping otherwise we can end up with big angle differences. I think this is the reason why we see higher chi2 in the ref/obs plot

[felix-russo]

Which params are not pi/2pi wrapped?

We should also wrap linearizedTrkParams, but trkParams comes from the propagation to the PCA - so there the angles should be in the correct ranges

[andiwand]

linearizedTrackParameters is not wrapped and paramDiff = trkParams - linearizedTrackParameters can also be out of range

this could be the reason why the chi2 gets bigger in the monitoring

not wrapping the angle here might be fine after all because we only check the difference at the end which is hopefully small after the update anyways

[felix-russo]

Sorry, but I don’t follow…
Before:

• newTrkMom was not wrapped
• linearizedTrkParams was not wrapped
• trkParams was wrapped

With the proposed change:

• newTrkMom is wrapped
• linearizedTrkParams is not wrapped
• trkParams is wrapped

Don’t we subtract unwrapped from wrapped in both cases?

Wouldn’t it be best to wrap all three quantities?

[andiwand]

but linearizedTrackParameters depends on newTrkMom which is now wrapped but was not wrapped before. this will then also influence the subtraction

we should either wrap everything or rely on the changes being small and ignore the wrapping (as it will be removed by the subtraction I think?)

[felix-russo]

I am not sure if we can ignore the wrapping:

If trkParams (which is wrapped) has an azimuthal angle of epsilon and linearizedTrkParams (which is not wrapped) has an azimuthal angle of 2pi + epsilon, they should exactly align.

If we then subtract, however, we get a contribution ~2pi/var(phi) to the chi2. Or am I missing something?

[andiwand]

If we then subtract, however, we get a contribution ~2pi/var(phi) to the chi2. Or am I missing something?

that would be the case after this change but the chi2 contribution should be 0 no? because they actually represent the same direction

[felix-russo]

I think the undesired contribution to the chi2 would arise both with and without this change! Or am I missing shtg?

The code is not ready to go in, I am just trying to get on the same page before I update it

[andiwand]

in principle I guess so yes but at the same time we see more chi2 outliers in the comparison plot #2769 (comment) . the values missing from the peek end up overflowing on the right

[felix-russo]

Ok I guess since we don't understand this but the previous version of the code just works better, I suggest to just leave everything as is.

The PR just removes the TODO now, should we get this in? @andiwand @paulgessinger