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: consider only tracks with non-zero weight in VertexPerfomanceWriter #2775

Merged

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented Dec 6, 2023

Previously, every track in TrackAtVertex contributed to the number of tracks on the reconstructed vertex. This does not make a lot of sense, since some tracks have low weights (which means that they actually do not contribute). This PR excludes low-weight tracks from the histograms to give a better indication of the vertexing performance.

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Dec 6, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69290be) 48.85% compared to head (14c3f27) 48.85%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2775   +/-   ##
=======================================
  Coverage   48.85%   48.85%           
=======================================
  Files         486      486           
  Lines       28178    28175    -3     
  Branches    13292    13290    -2     
=======================================
  Hits        13766    13766           
+ Misses       4796     4794    -2     
+ Partials     9616     9615    -1     

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

@paulgessinger
Copy link
Member

paulgessinger commented Dec 6, 2023

Are the physmon failures expected? [nTracksRecoVtx](https://acts-herald.app.cern.ch/view/acts-project/acts/1097044433/amvf_seeded.html#nTracksRecoVtx) goes down a bit, which I guess makes sense?

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. Could you add a few words on the motivation of the change to the PR description?

@paulgessinger paulgessinger added this to the next milestone Dec 6, 2023
@felix-russo
Copy link
Contributor Author

Are the physmon failures expected? [nTracksRecoVtx](https://acts-herald.app.cern.ch/view/acts-project/acts/1097044433/amvf_seeded.html#nTracksRecoVtx) goes down a bit, which I guess makes sense?

Yes! Before, every track in TrackAtVertex contributed to the number of tracks on the reconstructed vertex. Now, only tracks with weight >= 0.1 contribute; therefore, the number of reconstructed tracks decreases!

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Dec 15, 2023
@paulgessinger
Copy link
Member

I updated the references, but they might have picked up conflicts.

@felix-russo
Copy link
Contributor Author

CI is green now - can we get this in @paulgessinger?

@paulgessinger
Copy link
Member

@andiwand cpuld you approve this?

@kodiakhq kodiakhq bot merged commit ac5823f into acts-project:main Dec 15, 2023
52 checks passed
@acts-project-service
Copy link
Collaborator

✅ Athena integration test results [ac5823f]

✅ All tests successful

status job report
🟢 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_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 paulgessinger modified the milestones: next, v32.0.0 Jan 19, 2024
@felix-russo felix-russo deleted the save-nTracks-with-nonzero-weight branch January 20, 2024 13:53
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
…ceWriter (acts-project#2775)

Previously, every track in `TrackAtVertex` contributed to the number of tracks on the reconstructed vertex. This does not make a lot of sense, since some tracks have low weights (which means that they actually do not contribute). This PR excludes low-weight tracks from the histograms to give a better indication of the vertexing performance.

Co-authored-by: Paul Gessinger <1058585+paulgessinger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Examples Affects the Examples module Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants