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: AMVF error handling and zt splitting #2828

Merged
merged 20 commits into from
Mar 5, 2024

Conversation

felix-russo
Copy link
Contributor

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

  • Throws errors when encountering singular/non positive definite covariance matrices
  • Implements splitting in z-t

@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Vertexing labels Dec 13, 2023
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

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

Project coverage is 48.63%. Comparing base (fd0e845) to head (112f91d).

Files Patch % Lines
Core/src/Vertexing/AdaptiveMultiVertexFinder.cpp 13.51% 18 Missing and 14 partials ⚠️
Core/src/Vertexing/VertexingError.cpp 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2828      +/-   ##
==========================================
- Coverage   48.67%   48.63%   -0.05%     
==========================================
  Files         493      493              
  Lines       29004    29030      +26     
  Branches    13816    13844      +28     
==========================================
+ Hits        14117    14118       +1     
- Misses       4947     4965      +18     
- Partials     9940     9947       +7     

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

@felix-russo felix-russo changed the title refactor: functionality that determines whether vertices are merges refactor: functionality that determines whether vertices are merged Dec 15, 2023
@felix-russo felix-russo changed the title refactor: functionality that determines whether vertices are merged refactor!: functionality that determines whether vertices are merged Dec 15, 2023
@felix-russo felix-russo marked this pull request as ready for review December 15, 2023 17:18
@github-actions github-actions bot added the Stale label Jan 14, 2024
@github-actions github-actions bot removed the Stale label Jan 16, 2024
@github-actions github-actions bot removed the Stale label Feb 26, 2024
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.

Good stuff! I somehow overlooked that PR.

I worry a bit about the error log statements. Generally I am in favor of these but they backfired in the past because Athena is producing job failures if ACTS logs an error and this is usually not what we want.

We can either hope that they do not pop up or we lower the level a bit

@andiwand
Copy link
Contributor

This also conflicted with the vertexing refactor - do you think you can work on this over the next few days @felix-russo ? If not we can figure out how to follow up on this

@felix-russo
Copy link
Contributor Author

Thanks for the feedback!

I can work on this next weekend. If that's too late, feel free to reopen your PR or adapt this one; I don't mind which one goes in :)

@andiwand
Copy link
Contributor

resolved the conflicts and incorporated the feedback

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Mar 1, 2024
@andiwand andiwand changed the title refactor!: functionality that determines whether vertices are merged refactor: AMVF error handling and zt splitting Mar 1, 2024
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Mar 1, 2024
@andiwand andiwand removed Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Mar 2, 2024
@andiwand
Copy link
Contributor

andiwand commented Mar 2, 2024

looks like athena is fine with this

@paulgessinger
Copy link
Member

The GitLab physmon job fails for some reason that is not obvious to me.

kodiakhq bot pushed a commit that referenced this pull request Mar 4, 2024
Pulled this out of #2828 to encapsulate the breaking change

credits @felix-russo
@andiwand
Copy link
Contributor

andiwand commented Mar 4, 2024

Looks like this was an FPE that slipped through now. Moved the mask a bit. Let's see if that helps

@kodiakhq kodiakhq bot merged commit 6cc2c88 into acts-project:main Mar 5, 2024
54 checks passed
@github-actions github-actions bot removed the automerge label Mar 5, 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
Pulled this out of acts-project#2828 to encapsulate the breaking change

credits @felix-russo
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Mar 7, 2024
- Throws errors when encountering singular/non positive definite covariance matrices
- Implements splitting in `z`-`t`

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
Pulled this out of acts-project#2828 to encapsulate the breaking change

credits @felix-russo
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
- Throws errors when encountering singular/non positive definite covariance matrices
- Implements splitting in `z`-`t`

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
Pulled this out of acts-project#2828 to encapsulate the breaking change

credits @felix-russo
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
- Throws errors when encountering singular/non positive definite covariance matrices
- Implements splitting in `z`-`t`

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
Component - Core Affects the Core module Component - Examples Affects the Examples module Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants