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!: Changes to SeedFinderOrthogonal to allow reuse of the object #1757

Conversation

tboldagh
Copy link
Contributor

@tboldagh tboldagh commented Dec 16, 2022

This PR applies similar changes to these in #1693 . As a result the SeedFinderOrthogonal does not need to be recreated for every event.

Closes #1690

BREAKING CHANGE
Method to crate OthogonalSeedFinder and createSeeds are modified. The former does not take SeedOptions while the later does now.

@tboldagh tboldagh added Component - Core Affects the Core module Improvement Changes to an existing feature labels Dec 16, 2022
@tboldagh tboldagh added this to the next milestone Dec 16, 2022
@github-actions
Copy link

github-actions bot commented Dec 16, 2022

📊 Physics performance monitoring for ee3424b

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 Dec 16, 2022

Codecov Report

Merging #1757 (5b5b4ab) into main (fabf9ac) will not change coverage.
The diff coverage is n/a.

❗ Current head 5b5b4ab differs from pull request most recent head ee3424b. Consider uploading reports for the commit ee3424b to get more accurate results

@@           Coverage Diff           @@
##             main    #1757   +/-   ##
=======================================
  Coverage   49.59%   49.59%           
=======================================
  Files         407      407           
  Lines       22600    22600           
  Branches    10305    10305           
=======================================
  Hits        11208    11208           
  Misses       4211     4211           
  Partials     7181     7181           

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

Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Looks like an excellent change.

Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member

Ah, the CI doesn't run while there are conflicts. After the conflicts have been resolved, you'd be able to pull the ROOT hash from the CI to update.

@tboldagh
Copy link
Contributor Author

Ah, the CI doesn't run while there are conflicts. After the conflicts have been resolved, you'd be able to pull the ROOT hash from the CI to update.

I have to say that the gitlab CI visualisation is way simpler to digest.

@stephenswat
Copy link
Member

Hi @tboldagh, is this ready to go? I'm excited to merge it in. 😄

@tboldagh
Copy link
Contributor Author

Hi @tboldagh, is this ready to go? I'm excited to merge it in. 😄

Yes. However I am still GitHub-dumb and not sure if all is good. I.e. there are changes im plots (like the one attached) that the CI did not complain about:
image

@paulgessinger
Copy link
Member

The physmon histogram comparison tries to be smart and doesn't check them for bit-identicality (somewhat similar to dcube).

Is this histogram from the reference file you updated?

@tboldagh
Copy link
Contributor Author

No, Another one. (Orthogonal seeding.) The updated one match perfectly.
image

@paulgessinger
Copy link
Member

Ok so a change there is expected. You can either update that reference file as well or we merge as is.

@tboldagh
Copy link
Contributor Author

Ok so a change there is expected. You can either update that reference file as well or we merge as is.

I think it is better to update to reduce other people confusion. Doing that now.

@kodiakhq kodiakhq bot merged commit 77b55d0 into acts-project:main Jan 18, 2023
@paulgessinger paulgessinger modified the milestones: next, v23.0.0 Jan 18, 2023
kodiakhq bot pushed a commit that referenced this pull request Feb 1, 2023
Due to @tboldagh's recent changes (#1757) and the significant reduction in the number of seeds produced by the orthogonal seeding, I noticed an error in the calculation of interactionPointCut that resulted in a reduction in efficiency for |eta| > 3 in the ITk examples. This PR fixes the problem.

Before:
![ITkSeeding_ttbar_mu0_PPP_trackeff_vs_eta copy](https://user-images.githubusercontent.com/56648068/215765194-54a093f0-1e01-4860-8111-e60a99ce9eb6.png)
After:
![ITkSeeding_ttbar_mu0_PPP_trackeff_vs_eta](https://user-images.githubusercontent.com/56648068/215765247-ba318582-8d2b-4344-bb82-c42cbcf792b9.png)
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 16, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 17, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 25, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 25, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 25, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 26, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 30, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 5, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 6, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 15, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 15, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 17, 2023
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 Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Seeding to avoid repeated Finders instantiations
4 participants