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: Adjust full_chain_itk configuration for updated vertex fitting algori… #1636

Closed
wants to merge 1 commit into from
Closed

fix: Adjust full_chain_itk configuration for updated vertex fitting algori… #1636

wants to merge 1 commit into from

Conversation

goetzgaycken
Copy link
Contributor

The vertex fitting algorithm does not perform a track selection anymore (since #1579) but e.g. expects preselected track parameters as input.

Adjust full_chain_itk configuration for updated vertex fitting algori…
3746b08 
…thm.

The vertex fitting algorithm does not perform a track selection anymore
but expects preselected track parameters as input.
@andiwand
Copy link
Contributor

hey @goetzgaycken ! sorry I forgot to follow up with the itk full chain. you can attach the track selection to the track finding now. an example is given in the odd full chain here

@AJPfleger AJPfleger added this to the next milestone Oct 31, 2022
@goetzgaycken
Copy link
Contributor Author

you can attach the track selection to the track finding now.

  • Sure. But in run2/3 the track selection for vertex finding was tighter than for track finding. Is that different for run4 ?
  • Would that do something different than the track selector algorithm concerning the vertex finding ?

@AJPfleger AJPfleger added Impact - Minor Nuissance bug and/or affects only a single module Component - Examples Affects the Examples module labels Oct 31, 2022
@paulgessinger
Copy link
Member

Hm, at least we could experiment with a tighter track selection before the vertexing. Thoughts @timadye ?

@andiwand
Copy link
Contributor

I moved the track selection into CKF because I thought it aligns better with the seeding interface for example. I did not consider any procedure from run2/3/4. I can add the track selection back into the vertexing if that is a requirement. But I guess the track finding should keep the track selection?

@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Merging #1636 (3746b08) into main (5c07814) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1636   +/-   ##
=======================================
  Coverage   48.65%   48.65%           
=======================================
  Files         381      381           
  Lines       20780    20780           
  Branches     9518     9518           
=======================================
  Hits        10110    10110           
  Misses       4099     4099           
  Partials     6571     6571

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

@github-actions
Copy link

📊 Physics performance monitoring for 3746b08

Full report
CKF: seeded, truth smeared, truth estimated
IVF: seeded, truth smeared, truth estimated
Ambiguity resolution
Truth tracking

Vertexing

IVF seeded

IVF truth smeared

IVF truth estimated

CKF

seeded

truth smeared

truth estimated

Ambiguity resolution

seeded

Truth tracking

Truth tracking

@timadye
Copy link
Contributor

timadye commented Oct 31, 2022

I think it would be good to add the TrackSelectorRanges option back into addVertexFitting. Sorry I hadn't spotted that it was removed by #1579. We should be able to specify the selection in addCKFTracks or addVertexFitting, or both (as @goetzgaycken suggests), even if our examples plump for one or the other.

Can we include addAmbiguityResolution in the full_chain_itk.py example?

@andiwand
Copy link
Contributor

makes sense. sorry for the back and forth. having two track selectors makes the wiring a bit complicated so I would wait for #1588 to clean this up

This was referenced Nov 1, 2022
Merged
Merged
kodiakhq bot pushed a commit that referenced this pull request Nov 1, 2022
@andiwand
fix: ITk full chain (#1642)
c780eb2 
- #1636
- fix #1641 
- align ITk full chain with ODD full chain
  - add ambiguity resolution
  - catch up with interface changes of `addCKFTracks` and `addVertexFitting`
kodiakhq bot pushed a commit that referenced this pull request Nov 1, 2022
@andiwand
feat: examples restore vertexing track selector (#1644)
e1b8ebb 
#1579 remove the track selector from `addVertexing` which is still required. see #1636

#1588 should clean this up a little
@goetzgaycken
Copy link
Contributor Author

closing this, because problem fixed by #1642 (which adds ambiguity resolution which provides track selection)

@paulgessinger paulgessinger modified the milestones: next, v21.0.0 Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Component - Examples Affects the Examples module Impact - Minor Nuissance bug and/or affects only a single module
Projects
None yet
Milestone
v21.0.0
Development

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