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

perf: use analytical solution for straight tracks during impact point estimation #2506

Merged
merged 10 commits into from
Oct 7, 2023

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented Oct 3, 2023

We use the Newton method to find the 3D PCA for helical tracks. Straight tracks are currently approximated by setting the helix radius to a very large value.

In this PR, the approximation is replaced by an analytical solution (which exists for straight tracks). This should lead to a better performance and (slightly) more precise results. To check the analytical solution, a unit test is added.

A derivation of the analytical solution can be found in the updated reference: Track_Linearization_and_3D_PCA.pdf

@felix-russo felix-russo changed the title perf: analytical solution for straight tracks perf: use analytical solution for straight tracks for impact point estimation Oct 3, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Oct 3, 2023
@felix-russo felix-russo changed the title perf: use analytical solution for straight tracks for impact point estimation perf: use analytical solution for straight tracks during impact point estimation Oct 4, 2023
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #2506 (799b25b) into main (020a076) will decrease coverage by 0.01%.
The diff coverage is 30.43%.

@@            Coverage Diff             @@
##             main    #2506      +/-   ##
==========================================
- Coverage   49.77%   49.77%   -0.01%     
==========================================
  Files         466      466              
  Lines       26298    26310      +12     
  Branches    12084    12093       +9     
==========================================
+ Hits        13091    13095       +4     
+ Misses       4617     4616       -1     
- Partials     8590     8599       +9     
Files Coverage Δ
...re/include/Acts/Vertexing/ImpactPointEstimator.hpp 100.00% <ø> (ø)
...re/include/Acts/Vertexing/ImpactPointEstimator.ipp 36.89% <30.43%> (-0.23%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@felix-russo felix-russo marked this pull request as ready for review October 5, 2023 08:33
@andiwand andiwand added this to the next milestone Oct 6, 2023
@kodiakhq kodiakhq bot merged commit d8c447c into acts-project:main Oct 7, 2023
@github-actions github-actions bot removed the automerge label Oct 7, 2023
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 7, 2023
@paulgessinger
Copy link
Member

The actual output checks on the failed jobs seem to succeed:

INFO     ---------------------------------------------------------------------------------------
INFO     Running q442 Frozen Tier0 Policy Check on AOD for 60 events
INFO     Checking for reference override at https://actseos.web.cern.ch/ci/atlas/WorkflowReferences/main/q442/v24/myAOD.pool.root
INFO     No reference override found
INFO     Reading the reference file from location /cvmfs/atlas-nightlies.cern.ch/repo/data/data-art/WorkflowReferences/main/q442/v24/myAOD.pool.root
INFO     Neither '/builds/acts/acts-athena-ci/run/run_q442/q442_AOD_diff-exclusion-list.txt' nor '/builds/acts/acts-athena-ci/run/run_q442/q442_AOD_diff-interest-list.txt' files exist in the release.
INFO     No diff rules file exists, using the default list
INFO     Passed!

INFO     ---------------------------------------------------------------------------------------
INFO     Running q442 Frozen Tier0 Policy Check on ESD for 20 events
INFO     Checking for reference override at https://actseos.web.cern.ch/ci/atlas/WorkflowReferences/main/q442/v24/myESD.pool.root
INFO     No reference override found
INFO     Reading the reference file from location /cvmfs/atlas-nightlies.cern.ch/repo/data/data-art/WorkflowReferences/main/q442/v24/myESD.pool.root
INFO     Reading the diff rules file from location /builds/acts/acts-athena-ci/run/run_q442/q442_ESD_diff-exclusion-list.txt
INFO     Passed!

But then the q442 AOD metadata checks reports a mismatch. However, I don't see how this could be caused by these changes.

Also, this exact job failed with this exact difference also after #2460 went in, but then succeeded when #2516 was merged.

Could be an unrelated issue?

@paulgessinger
Copy link
Member

When retrying, the job still fails (maybe something about the build is off?) but when I trigger a clean job it succeeds. I think we're good.

@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Oct 7, 2023
@felix-russo felix-russo deleted the ipe-straight-tracks branch October 8, 2023 16:20
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 10, 2023
@paulgessinger paulgessinger modified the milestones: next, v30.2.0 Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants