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

fix: Wire optional seed result in TrackDensityVertexFinder after #2917 #3002

Merged

Conversation

andiwand
Copy link
Contributor

In #2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the TrackDensityVertexFinder correctly.

@andiwand andiwand added this to the next milestone Feb 29, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Feb 29, 2024
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Feb 29, 2024
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

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

Project coverage is 48.69%. Comparing base (3d96535) to head (8e1ac39).

Files Patch % Lines
Core/src/Vertexing/GaussianTrackDensity.cpp 30.76% 9 Missing ⚠️
Core/src/Vertexing/TrackDensityVertexFinder.cpp 57.14% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3002      +/-   ##
==========================================
- Coverage   48.69%   48.69%   -0.01%     
==========================================
  Files         493      493              
  Lines       28980    28991      +11     
  Branches    13800    13803       +3     
==========================================
+ Hits        14112    14117       +5     
- Misses       4939     4946       +7     
+ Partials     9929     9928       -1     

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

@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Feb 29, 2024
@paulgessinger
Copy link
Member

I think the output failures indicate that we're going back to the original outputs. I'd be inclined to either merge this and then manually remove the reference overrides (thus going back to the vanilla references active in Athena main) or doing that before merging and triggering here again. In any case, it's a bit of a manually timed operation.

paulgessinger
paulgessinger previously approved these changes Feb 29, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Mar 1, 2024

I think the output failures indicate that we're going back to the original outputs. I'd be inclined to either merge this and then manually remove the reference overrides (thus going back to the vanilla references active in Athena main) or doing that before merging and triggering here again. In any case, it's a bit of a manually timed operation.

What suits better for you? From my side both is fine. If you revert the output changes we can just make this PR the next to go in

@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 1, 2024
@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels 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
@paulgessinger
Copy link
Member

Retriggered now with the reference override from before #2917. There were also two failure just due to connection issues, I believe.

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.

Let's go!

@kodiakhq kodiakhq bot merged commit abdddc6 into acts-project:main Mar 1, 2024
54 checks passed
@github-actions github-actions bot removed the automerge label Mar 1, 2024
@andiwand andiwand deleted the fix-trackdensityvertexfinder-after-2917 branch March 1, 2024 17:50
@paulgessinger paulgessinger modified the milestones: next, v33.0.0 Mar 6, 2024
@paulgessinger paulgessinger added Fails Athena tests This PR causes a failure in the Athena tests and removed Fails Athena tests This PR causes a failure in the Athena tests labels Mar 6, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Mar 7, 2024
…ts-project#2917 (acts-project#3002)

In acts-project#2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…ts-project#2917 (acts-project#3002)

In acts-project#2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…ts-project#2917 (acts-project#3002)

In acts-project#2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.
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 Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ... Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants