Skip to content

Conversation

@wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Jan 12, 2026

Briefly, what does this PR introduce?

This PR upgrades to acts@44.4.0 and ensures that we use acts +edm4hep which is what now contains the ActsPodioEdm podio data model definitions.

Note: the upgrade of acts to 44.4.0 is required because acts-project/acts#4820 fixes EDM4hepUtil.cpp to use edm4hep::CovMatrix6f brace initializers.

Needs:

Copilot AI review requested due to automatic review settings January 12, 2026 03:42

This comment was marked as resolved.

@wdconinc

This comment was marked as resolved.

@wdconinc

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings January 12, 2026 16:03

This comment was marked as resolved.

@wdconinc wdconinc changed the title fix: acts +edm4hep chore: acts@44.4.0 +edm4hep Jan 12, 2026
@wdconinc wdconinc requested a review from veprbl January 12, 2026 16:45
@wdconinc wdconinc enabled auto-merge (squash) January 12, 2026 17:27
@veprbl
Copy link
Member

veprbl commented Jan 13, 2026

Capybara is not happy: https://veprbl.github.io/capybara-reports/458fc303024e08f06439055c3579ce03/
there are changes to trajectories' nHoles, nOutliers as well as nStates. We probably should understand this better before we move forward with this.

@wdconinc wdconinc disabled auto-merge January 13, 2026 03:40
@wdconinc
Copy link
Contributor Author

Strange. acts-project/acts@v44.3.0...v44.4.0 looks harmless.

@veprbl
Copy link
Member

veprbl commented Jan 14, 2026

The difference is introduced by acts-project/acts@6fb4b10

https://veprbl.github.io/capybara-reports/494fdd3dd92c2b93892ada4fdc7a0619/ is the comparisons between adjacent commits

@wdconinc
Copy link
Contributor Author

The heck?!?

@wdconinc
Copy link
Contributor Author

acts-project/acts#4858

@wdconinc
Copy link
Contributor Author

acts-project/acts#4858

The ACTS issue actually fixes a bug that affected us (but we didn't know) for any TGeoTubeSeg placement of a tracking detector with a placement around 0 degrees (only MPGDInnerTracker). So it seems this is a change for the better.

We have some TGeoTubeSeg we place around 0 degrees, -dphi/2 to +dphi/2 style. These are given a phi = [-23.74,23.74] deg in the dd4hep Tube creation (i.e. we pass those values scaled in radians).

ROOT turns this into phi=[336.26,383.74] deg (in actual degrees), which previously (< v44.4) were then read by ACTS as [-0.41,6.70] old rad. With the fix this becomes (correctly) [-0.41,0.41] new rad.

Detailed logging output with the patch MPGDCylinderBarrelTracker_geo.patch against eic/epic@1f93f05

MPGDCylinderBarrelTracker INFO  Stave Model #0,"MMInnerSector": rmin = 55.50 cm  phi = [-23.74,23.74] deg
MPGDCylinderBarrelTracker INFO    Outward Frame Tube: phi=[336.26,383.74] deg = [-0.41,6.70] old rad = [-0.41,0.41] new rad
MPGDCylinderBarrelTracker INFO    Start/Stop -24.776553 -23.744197
MPGDCylinderBarrelTracker INFO    Start/Stop 23.744197 24.776553
MPGDCylinderBarrelTracker INFO  Stave Model #1,"MMOuterSector": rmin = 57.70 cm  phi = [-22.84,22.84] deg
MPGDCylinderBarrelTracker INFO    Outward Frame Tube: phi=[337.16,382.84] deg = [-0.40,6.68] old rad = [-0.40,0.40] new rad
MPGDCylinderBarrelTracker INFO    Start/Stop -23.831867 -22.838872
MPGDCylinderBarrelTracker INFO    Start/Stop 22.838872 23.831867
MPGDCylinderBarrelTracker INFO    Service to inner sector: rmin = 57.65 cm thickness = 0.05 cm length = 56.65 cm phi = [-21.55,21.55] deg

So, in summary, I think this is actually a change we should merge since it fixes an underlying issue.

@wdconinc
Copy link
Contributor Author

Of course (since I buried the lede), the reason why this adds more holes to trajectories is because the cylinder surface bounds of the surface cover the entire phi circumference and then some. Every track will have a hole in that surface.

@wdconinc
Copy link
Contributor Author

@mposik1983 FYI analysis of tracking issue affecting the MPGD inner barrel.

@wdconinc
Copy link
Contributor Author

If we upgrade to ACTS v44.4.0, we will want to pick up acts-project/acts#4968 too (as a patch or as a potential v44.4.1) since it will affect our CI builds with gcc and clang.

Copilot AI review requested due to automatic review settings January 19, 2026 00:49
@wdconinc wdconinc enabled auto-merge (squash) January 19, 2026 00:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the tracking meeting and on Acts Mattermost, this is in fact fixing a bug.

@wdconinc wdconinc merged commit 18bad8f into master Jan 19, 2026
29 checks passed
@wdconinc wdconinc deleted the acts-edm4hep branch January 19, 2026 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants