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

feat: save vertex seed #2885

Merged
merged 20 commits into from
Mar 6, 2024
Merged

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented Jan 20, 2024

Saves 4D of the vertex seed (its x- and y- coordinate are not estimated in current seeding algorithms and should be set to 0).

@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Vertexing labels Jan 20, 2024
@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Jan 20, 2024
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Attention: Patch coverage is 58.82353% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 48.67%. Comparing base (b7f6cc0) to head (3e35e28).

Files Patch % Lines
Core/src/Vertexing/Vertex.cpp 58.82% 9 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2885      +/-   ##
==========================================
- Coverage   48.68%   48.67%   -0.01%     
==========================================
  Files         493      493              
  Lines       29053    29055       +2     
  Branches    13854    13853       -1     
==========================================
- Hits        14144    14143       -1     
- Misses       4963     4966       +3     
  Partials     9946     9946              

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

@paulgessinger paulgessinger added this to the next milestone Jan 22, 2024
@andiwand
Copy link
Contributor

that's a good idea!

at some point we may want to split this performance writer because it already does a couple of things at the same time. but this is not relevant for you

@felix-russo
Copy link
Contributor Author

Do you have any idea why recoOverAcc, recoOverRecbl, and recoOverTrue differ in the physmon?

These quantities are just divisions of nTrueVtx, nRecoVtx, and nTrueVtx, etc., which align exactly...

I am quite puzzled @andiwand @paulgessinger

@paulgessinger
Copy link
Member

@felix-russo i think they're reconstructed vertices over all vertices, reconstructible only and vertices in the acceptance only.

So I don't think they should be identical.

@felix-russo
Copy link
Contributor Author

Found the mistake 🥳

@paulgessinger
Copy link
Member

And are you sure they're different? I think the jobs are just red because the new histograms are missing in the references.

@felix-russo
Copy link
Contributor Author

And are you sure they're different? I think the jobs are just red because the new histograms are missing in the references.

The mistake was in the previous versions of the code. 5 minutes before you commented the physmon was updated and looks fine now! Thanks for looking at this on the weekend :) @paulgessinger

@felix-russo
Copy link
Contributor Author

Here are some histograms of the seed coordinates and of the residuals for amvf + gridseeder seeded. The seed's x- and y-coordinate are 0 as expected (the mean value of 0.0006 comes because the central bin is not exactly at 0). Marking this PR as ready for review...
image
image
image
image
image
image

@felix-russo felix-russo marked this pull request as ready for review January 27, 2024 16:40
@github-actions github-actions bot added the Component - Fatras Affects the Fatras module label Mar 1, 2024
@andiwand
Copy link
Contributor

andiwand commented Mar 1, 2024

resolved the conflicts - waiting for physmon to complete to update refs

@kodiakhq kodiakhq bot merged commit 2664480 into acts-project:main Mar 6, 2024
54 checks passed
@github-actions github-actions bot removed the automerge label Mar 6, 2024
@paulgessinger paulgessinger modified the milestones: next, v33.0.0 Mar 6, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Mar 7, 2024
Saves 4D of the vertex seed (its x- and y- coordinate are not estimated in current seeding algorithms and should be set to 0).

Co-authored-by: Andreas Stefl <487211+andiwand@users.noreply.github.com>
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
Saves 4D of the vertex seed (its x- and y- coordinate are not estimated in current seeding algorithms and should be set to 0).

Co-authored-by: Andreas Stefl <487211+andiwand@users.noreply.github.com>
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
Saves 4D of the vertex seed (its x- and y- coordinate are not estimated in current seeding algorithms and should be set to 0).

Co-authored-by: Andreas Stefl <487211+andiwand@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 - Core Affects the Core module Component - Examples Affects the Examples module Infrastructure Changes to build tools, continous integration, ... Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants