-
Notifications
You must be signed in to change notification settings - Fork 1
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
[DO NOT MERGE] Single iteration Patatrack and LST in HLT #94
[DO NOT MERGE] Single iteration Patatrack and LST in HLT #94
Conversation
…k seeds for Phase 2 HLT
…nd triplets as input
…akaPatatrackInHLT_LSTInHLT_actualSingleIterPatatrack_latest Single Iteration Patatrack and Single Iteration Patatrack-only-seeded LST in HLT
constexpr unsigned int n_max_pixel_track_candidates = 30000; | ||
constexpr unsigned int n_max_nonpixel_track_candidates = 1000; | ||
constexpr unsigned int n_max_pixel_track_candidates = 3000000; | ||
constexpr unsigned int n_max_nonpixel_track_candidates = 100000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the cost to the memory used from changes in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure.
In the final PR, these increase "only" by 10x (and I can probably fine-tune it to something less). They are also needed only in the case of seedingLST
, which I would not use at this stage. Maybe I should not touch them and revisit if/when the seedingLST
config becomes usable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm partly confused what "final PR" means. Does it mean that at least this PR target branch should be updated or the PR itself to be rebased?
I think https://docs.google.com/spreadsheets/d/1-tDK4nIPlrB-0SoeEbnJlf_lyt-xTZcIhmZGRnR-fnY/edit?gid=16484832#gid=16484832 is still mostly up to date at least for memory per object. So, just the expected memory use would be enough to clarify my question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm partly confused what "final PR" means. Does it mean that at least this PR target branch should be updated or the PR itself to be rebased?
Also the PR needs to be rebased. Small adjustments but it needs to. The target branch will also change - should I just switch to SegmentLinking/master
, after I sync it with the upstream, or will that cause issues?
I think https://docs.google.com/spreadsheets/d/1-tDK4nIPlrB-0SoeEbnJlf_lyt-xTZcIhmZGRnR-fnY/edit?gid=16484832#gid=16484832 is still mostly up to date at least for memory per object. So, just the expected memory use would be enough to clarify my question.
The spreadsheet was not up to date. I updated the default numbers and I get:
- size = 1208.8
- cache size = 1503.8
When using the values for the upcoming PR (the latter two 10x less than what is there in this PR):
n_max_pixel_segments_per_module = 500000;
n_max_pixel_track_candidates = 300000;
n_max_nonpixel_track_candidates = 10000;
I get:
- size = 1274.6
- cache size = 1581.1
I.e. ~5% increase in size and cache size. I will post the same numbers in PR description of the new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target branch will also change - should I just switch to
SegmentLinking/master
, after I sync it with the upstream, or will that cause issues?
master should be OK as a target, but please start the topic branch from some pre-release (pre4?) if possible. This way it should be easy to add it also to an LST development branch once we converge on the name and merge-base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I started from CMSSW_14_2_0_pre4
(made it clear also in the branch name and the PR description).
Closing this PR in favor of PR #132. |
PR description
This is a procModifier-based implementation of LST in HLT, based on top of
CMSSW_14_1_0_pre7
+ cms-sw#45117 + cms-sw#45508. The target branch has been adapted accordingly to be able to make a clear comparison.It is an adaptation/improvement over #63, which becomes moot in terms of LST (also mkFit would need to be re-implemented with the usage of procModifiers from that PR).
It tries to reuse the
trackingLST
procModifier, and it introduces a new one,seedingLST
, which may become also useful for the offline setup in the near future.The PR is not meant to be merged, as it will have to be rebased to whichever CMSSW version LST gets integrated. However, the developments are fairly decoupled from the rest of the framework and the rest of the LST code, so this PR should already be good enough for people to take a look and comment on the implementation details.
In terms of performance, this is shown in the "Tests on LST Configurations - Baseline vs. Patatrack+Legacy-seeded LST in InitialStep" section of this presentation.
Instructions on how to run