search

cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

Tracklet Processor Displaced #159

Closed hardikroutray closed 2 years ago

hardikroutray commented 2 years ago

PR description:

This PR introduces Tracklet Processor Displaced which combines the current displaced tracking functionality (Tracklet Engine Displaced, Triplet Engine and TrackletCalculatorDisplaced) into a single module

PR validation:

Efficiency checks with Displaced Mu samples show agreement between original and combined configuration. Presented in L1TkAlgo Meeting on 17 May. Also see introductory talk here

tomalin commented 2 years ago

Why is this PR changing the displaced tracklet wiring files in TrackFindingTracklet/data/ ?

data/ files are not put in the CMSSW repo, instead being stored in https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git . They then automatically become available in $CMSSW_RELEASE_BASE/external/ since CMSSW_12_0_0_pre4 . During development, we usually put such data/ files in https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet.git . But our git CI doesn't bother checking this out at the moment.

I suggest as a temporary work-around, you leave these data/ files where you have put them (if you do need to change them). This will allow git CI to pass. But then before we make a PR to central CMSSW, I'll delete them, and make a separate PR for them to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git .

tomalin commented 2 years ago

If not already done, please check performance OK on a 200PU sample, such as DisplacedMu + 200PU https://twiki.cern.ch/twiki/bin/view/CMS/L1TrackMC .

hardikroutray commented 2 years ago

If not already done, please check performance OK on a 200PU sample, such as DisplacedMu + 200PU https://twiki.cern.ch/twiki/bin/view/CMS/L1TrackMC .

Performance consistent between original and combined configuration for 200PU sample. Efficiency checks on slides 16,17 and 18 here

hardikroutray commented 2 years ago

Why is this PR changing the displaced tracklet wiring files in TrackFindingTracklet/data/ ?

data/ files are not put in the CMSSW repo, instead being stored in https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git . They then automatically become available in $CMSSW_RELEASE_BASE/external/ since CMSSW_12_0_0_pre4 . During development, we usually put such data/ files in https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet.git . But our git CI doesn't bother checking this out at the moment.

I suggest as a temporary work-around, you leave these data/ files where you have put them (if you do need to change them). This will allow git CI to pass. But then before we make a PR to central CMSSW, I'll delete them, and make a separate PR for them to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git .

Yes, data/ files need to be changed for the TPD module to work correctly. As you suggested, it would definitely be better if you could delete this and add these files to cms-data in a separate commit before PR to central CMSSW.

tomalin commented 2 years ago

Why is this PR changing the displaced tracklet wiring files in TrackFindingTracklet/data/ ? data/ files are not put in the CMSSW repo, instead being stored in https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git . They then automatically become available in $CMSSW_RELEASE_BASE/external/ since CMSSW_12_0_0_pre4 . During development, we usually put such data/ files in https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet.git . But our git CI doesn't bother checking this out at the moment. I suggest as a temporary work-around, you leave these data/ files where you have put them (if you do need to change them). This will allow git CI to pass. But then before we make a PR to central CMSSW, I'll delete them, and make a separate PR for them to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git .

Yes, data/ files need to be changed for the TPD module to work correctly. As you suggested, it would definitely be better if you could delete this and add these files to cms-data in a separate commit before PR to central CMSSW.

Please explain how the files in data/ have been changed. I believe these files for the displaced tracking are generated by the python scripts, such as HourGlassConfig.py that are documented in https://github.com/cms-L1TK/project_generation_scripts/blob/master/archive/README.md . But there are no new PRs from you changing these python scripts.

aehart commented 2 years ago

Why is this PR changing the displaced tracklet wiring files in TrackFindingTracklet/data/ ? data/ files are not put in the CMSSW repo, instead being stored in https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git . They then automatically become available in $CMSSW_RELEASE_BASE/external/ since CMSSW_12_0_0_pre4 . During development, we usually put such data/ files in https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet.git . But our git CI doesn't bother checking this out at the moment. I suggest as a temporary work-around, you leave these data/ files where you have put them (if you do need to change them). This will allow git CI to pass. But then before we make a PR to central CMSSW, I'll delete them, and make a separate PR for them to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git .

Yes, data/ files need to be changed for the TPD module to work correctly. As you suggested, it would definitely be better if you could delete this and add these files to cms-data in a separate commit before PR to central CMSSW.

Please explain how the files in data/ have been changed. I believe these files for the displaced tracking are generated by the python scripts, such as HourGlassConfig.py that are documented in https://github.com/cms-L1TK/project_generation_scripts/blob/master/archive/README.md . But there are no new PRs from you changing these python scripts.

@gershtein produced these files for us. I believe the process was basically to take the existing files for the extended tracking and replace each TCD, along with the TEDs and TREs that feed into it, with a TPD. Perhaps we can add any relevant scripts to project_generation_scripts next week when @gershtein gets back.

The ultimate plan is to update the TrackletConfigBuilder so that the wiring for the extended project can be generated dynamically, like that for the standard project is. But this will take a little longer.

tomalin commented 2 years ago

Why is this PR changing the displaced tracklet wiring files in TrackFindingTracklet/data/ ? data/ files are not put in the CMSSW repo, instead being stored in https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git . They then automatically become available in $CMSSW_RELEASE_BASE/external/ since CMSSW_12_0_0_pre4 . During development, we usually put such data/ files in https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet.git . But our git CI doesn't bother checking this out at the moment. I suggest as a temporary work-around, you leave these data/ files where you have put them (if you do need to change them). This will allow git CI to pass. But then before we make a PR to central CMSSW, I'll delete them, and make a separate PR for them to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git .

Yes, data/ files need to be changed for the TPD module to work correctly. As you suggested, it would definitely be better if you could delete this and add these files to cms-data in a separate commit before PR to central CMSSW.

Please explain how the files in data/ have been changed. I believe these files for the displaced tracking are generated by the python scripts, such as HourGlassConfig.py that are documented in https://github.com/cms-L1TK/project_generation_scripts/blob/master/archive/README.md . But there are no new PRs from you changing these python scripts.

@gershtein produced these files for us. I believe the process was basically to take the existing files for the extended tracking and replace each TCD, along with the TEDs and TREs that feed into it, with a TPD. Perhaps we can add any relevant scripts to project_generation_scripts next week when @gershtein gets back.

The ultimate plan is to update the TrackletConfigBuilder so that the wiring for the extended project can be generated dynamically, like that for the standard project is. But this will take a little longer.

Understood. We shouldn't really on hand-edited wiring maps in the data/ directory, since otherwise we wouldn't be able to update or validate them. So we do need @gershtein to make a separate PR to the project_generation_scripts directory , such that the scripts there can produce the new data/ files. (And yes, add this functionality to TrackletConfigBuilder in the longer term ...)

tomalin commented 2 years ago

Would the official CMSSW L1 track code be broken or damaged by your new TrackFindingTracklet/data/ files? i.e. Are your new data/ files reliant on any of the code change in this PR to keep the existing prompt & displaced tracking working OK? I'd like to understand if you can make a PR for your data/ files now to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git , without us needing to wait for cms-L1TK to be merged into official CMSSW first?

tomalin commented 2 years ago

@hardikroutray This PR looks almost ready to merge, aside from the unanswered comment about magic numbers.

hardikroutray commented 2 years ago

@hardikroutray This PR looks almost ready to merge, aside from the unanswered comment about magic numbers.

have answered it now.

hardikroutray commented 2 years ago

Would the official CMSSW L1 track code be broken or damaged by your new TrackFindingTracklet/data/ files? i.e. Are your new data/ files reliant on any of the code change in this PR to keep the existing prompt & displaced tracking working OK? I'd like to understand if you can make a PR for your data/ files now to https://github.com/cms-data/L1Trigger-TrackFindingTracklet.git , without us needing to wait for cms-L1TK to be merged into official CMSSW first?

The official CMSSW L1 track code shouldn't be affected by the files in TrackFindingTracklet/data/. Maybe @aehart can further comment on this.