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

refactor: make usage of vertex constraint more evident #2529

Closed

Conversation

felix-russo
Copy link
Contributor

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

Removes intricate use of constraint in AMVFinder

@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Oct 11, 2023
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (a8a0576) 48.68% compared to head (ae9dc2f) 49.53%.
Report is 43 commits behind head on main.

❗ Current head ae9dc2f differs from pull request most recent head e8f9d9c. Consider uploading reports for the commit e8f9d9c to get more accurate results

Files Patch % Lines
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp 9.09% 5 Missing and 5 partials ⚠️
...clude/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp 16.66% 0 Missing and 5 partials ⚠️
Core/include/Acts/Vertexing/AMVFInfo.hpp 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2529      +/-   ##
==========================================
+ Coverage   48.68%   49.53%   +0.85%     
==========================================
  Files         478      474       -4     
  Lines       27888    27013     -875     
  Branches    13172    12474     -698     
==========================================
- Hits        13578    13382     -196     
+ Misses       4765     4762       -3     
+ Partials     9545     8869     -676     

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

@paulgessinger paulgessinger added this to the next milestone Oct 23, 2023
andiwand
andiwand previously approved these changes Oct 23, 2023
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

code looks fine but physmon seems to be degraded

@felix-russo
Copy link
Contributor Author

code looks fine but physmon seems to be degraded

Yes, I didn't find the time to investigate yet, will do so shortly!

@acts-project-service
Copy link
Collaborator

acts-project-service commented Oct 25, 2023

🔴 Athena integration test results [e8f9d9c]

🔴 Some tests have failed!

Please investigate the pipeline!

status job
🟢 run_unit_tests
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsBenchmarkWithSpot.sh 8 100
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsWorkflow.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateAmbiguityResolution.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateResolvedTracks.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateTracks.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsCoreSpacePoints.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsSpacePoints.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateSeeds.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateOrthogonalSeeds.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateClusters.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsPersistifyEDM.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsGSFRefitting.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/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_data
🔴 run_workflow_tests_run3_mc
🔴 run_workflow_tests_run3_data
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco

@felix-russo felix-russo marked this pull request as ready for review November 28, 2023 17:03
@felix-russo felix-russo changed the title refactor: employ useConstraintInFit from VertexingOptions in AMVFitter refactor: make usage of vertex constraint more evident Nov 28, 2023
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Dec 1, 2023
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Dec 1, 2023
@felix-russo
Copy link
Contributor Author

This should not break athena, I will investigate

@felix-russo felix-russo marked this pull request as draft December 5, 2023 09:22
@paulgessinger
Copy link
Member

There also seems to be a unit test failure in AdaptiveMultiVertexFitter .

@paulgessinger
Copy link
Member

The changes seen in Athena seem pretty wide, I see taus, jets and muons being widely affected. Not sure about the scale yet.

@felix-russo
Copy link
Contributor Author

Yeah this should not affect athena! I will hopefully find the time to investigate soon, but this is not at all ready

@paulgessinger
Copy link
Member

This seems to change the chi2/ndf quite visibly:

https://acts-athena-outputs.web.cern.ch/test_data18_13TeV_1000evt/pulls/2529/dcube_expert_last/

image

image

@github-actions github-actions bot added the Stale label Jan 6, 2024
@felix-russo felix-russo deleted the useConstraintInFit-AMVFitter branch January 20, 2024 13:55
@paulgessinger paulgessinger modified the milestones: next, v32.1.0 Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module 👷‍♀️ User Action Needed Fails Athena tests This PR causes a failure in the Athena tests Stale Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants