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.

Sign up for GitHub

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

Jump to bottom

fix: Skip seeds with negative cotThetaAvg2 #1655

Merged
merged 9 commits into from
Dec 1, 2022
Merged

fix: Skip seeds with negative cotThetaAvg2 #1655

merged 9 commits into from
Dec 1, 2022

Conversation

paulgessinger
Copy link
Member

No description provided.

@paulgessinger paulgessinger added this to the next milestone Nov 4, 2022
@github-actions
Copy link

github-actions bot commented Nov 4, 2022 •
edited
Loading

📊 Physics performance monitoring for c05bfcd

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@codecov
Copy link

codecov bot commented Nov 4, 2022 •
edited
Loading

Codecov Report

Merging #1655 (47bda8f) into main (3701035) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

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

@@            Coverage Diff             @@
##             main    #1655      +/-   ##
==========================================
- Coverage   49.02%   49.01%   -0.01%     
==========================================
  Files         397      397              
  Lines       21558    21560       +2     
  Branches     9817     9818       +1     
==========================================
  Hits        10568    10568              
- Misses       4181     4183       +2     
  Partials     6809     6809
Impacted Files Coverage Δ
Core/include/Acts/Seeding/SeedFinder.ipp 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

andiwand
andiwand approved these changes Nov 7, 2022
View reviewed changes
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.

cool 👍 will see if clang-tidy header checks will call us out about it soon

Core/include/Acts/Seeding/SeedFinder.ipp Outdated Show resolved Hide resolved
@LuisFelipeCoelho
Copy link
Member

Shouldn't this be changed in the orthogonal seeding as well?

@paulgessinger
Copy link
Member Author

Probably, yeah

LuisFelipeCoelho
LuisFelipeCoelho approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@LuisFelipeCoelho LuisFelipeCoelho left a comment

Choose a reason for hiding this comment

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

Sorry, we don't have cotThetaAvg2 in the orthogonal seeding. Probably I will add it later for the seeding performance writer

@paulgessinger paulgessinger added automerge 🛑 blocked This item is blocked by another item and removed automerge labels Nov 15, 2022
@paulgessinger
Copy link
Member Author

Let's wait until #1665 is merged.

@paulgessinger paulgessinger removed the 🛑 blocked This item is blocked by another item label Nov 16, 2022
@paulgessinger
Copy link
Member Author

Seeding reference changes are expected I think. Here's a subset of plots from the residuals and pulls, which shows a few failures:
res.html.zip

I'm guessing this is mostly due to bin changes as the ranges are still not fixed. One thing that I find strange though is the change in all_res_t_prt:

image

@paulgessinger
Copy link
Member Author

CI passes now with updated reference.

@LuisFelipeCoelho ?

@LuisFelipeCoelho
Copy link
Member

looks good to me

@kodiakhq kodiakhq bot merged commit d3f0cc3 into acts-project:main Dec 1, 2022
@github-actions github-actions bot removed the automerge label Dec 1, 2022
pbutti pushed a commit to pbutti/acts that referenced this pull request Dec 13, 2022
@paulgessinger @pbutti
fix: Skip seeds with negative cotThetaAvg2 (acts-project#1655)
e33ade6
@paulgessinger paulgessinger modified the milestones: next, v22.0.0 Dec 21, 2022
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
@paulgessinger
fix: Skip seeds with negative cotThetaAvg2 (acts-project#1655)
35b6ce0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AJPfleger AJPfleger AJPfleger requested changes

@LuisFelipeCoelho LuisFelipeCoelho LuisFelipeCoelho approved these changes

@andiwand andiwand andiwand approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v22.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@paulgessinger @LuisFelipeCoelho @andiwand @AJPfleger @benjaminhuth