Refactoring of Phase1 OMTF emulator & Phase2 OMTF emulator #1196

mbluj · 2024-01-10T21:52:47Z

PR description:

This PR consists of two parts:

Refactoring of Phase1 OMTF emulator that is rebase of development in PR #43687 to the official CMSSW,
and Phase2 OMTF emulator code on top of it (added as a new CMSSW package).

Neither changes introduced for Phase1 nor Phase2 are switched on in standard workflows. For it dedicated PRs will be prepared.

PR validation:

Tested with matrix tests (limited set)

FYI, @kbunkow, @akalinow

triggerDoctor · 2024-01-11T08:10:58Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-01-11T08:14:59Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-01-11T08:15:09Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 2 files that did not meet formatting requirements:

L1Trigger/L1TMuonOverlapPhase1/src/Omtf/OMTFConfiguration.cc
L1Trigger/L1TTrackMatch/plugins/L1TrackJetClustering.h

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

mbluj · 2024-01-11T08:58:09Z

I can apply code formatting fixes, but please note that:
a) one of affected files has nothing to do with the OMTF emulator,
b) formatting issue in the OMTF-related file is due to difference in formatting rules between 13_3 and 14_0 (master) release series, i.e. the file passed formatting test as in 14_0.
So, should I proceed with the formatting fixes or it is fine to keep the code as it is?

mbluj · 2024-01-11T11:21:27Z

I can apply code formatting fixes, but please note that: a) one of affected files has nothing to do with the OMTF emulator, b) formatting issue in the OMTF-related file is due to difference in formatting rules between 13_3 and 14_0 (master) release series, i.e. the file passed formatting test as in 14_0. So, should I proceed with the formatting fixes or it is fine to keep the code as it is?

So, I applied the formatting fix and squashed with previous commit with a similar fix. After the squashing you can note that the fix touches only no-OMTF-related code as changes of OMTF code simply inverted previous change introduced for master/14_0...

aloeliger

I didn't go over the phase 1 stuff in too much detail, since I'm not sure how much of that is already present in CMSSW, or is otherwise not really negotiable here. If this is all new stuff and none of it is in CMSSW, I know there is a PR open to CMSSW, I will go read and review there.

This PR is still quite large otherwise, so this is not the most thorough review.

In general, please go and remove commented out code that isn't serving to demonstrate or clarify something in the code around it, or standing in for code to be implemented later.

L1Trigger/L1TMuonOverlapPhase1/src/AngleConverterBase.cc

L1Trigger/L1TMuonOverlapPhase1/src/Omtf/XMLEventWriter.cc

L1Trigger/L1TMuonOverlapPhase1/src/Tools/CandidateSimMuonMatcher.cc

L1Trigger/L1TMuonOverlapPhase1/src/Tools/DataROOTDumper2.cc

L1Trigger/L1TMuonOverlapPhase2/src/PtAssignmentNNRegression.cc

aloeliger · 2024-01-17T16:43:34Z

L1Trigger/L1TMuonOverlapPhase2/src/PtAssignmentNNRegression.cc

+          <<" meanDistPhiValue   "<<omtfCand->getGoldenPatern()->meanDistPhiValue(iLogicLayer, omtfCand->getRefLayer())//<<(phiDist != hit.phiDist? "!!!!!!!<<<<<" : "")
+          <<endl;*/
+
+      if (hit.phiDist > 504 || hit.phiDist < -512) {


Could these numbers be a bit better explained with some constants?

@kbunkow, can you take a look, please?

aloeliger · 2024-01-17T16:45:24Z

L1Trigger/L1TMuonOverlapPhase2/test/expert/runMuonOverlapPatternGenerator_SF.py

This looks similar to previous python files where it has user specific information in it. Do these need to be in CMSSW? Will it be future proof?

@kbunkow, can you take check expert config files for phase2, please?

aloeliger · 2024-01-17T16:46:18Z

L1Trigger/L1TMuonOverlapPhase2/test/expert/runMuonOverlapPatternGenerator_displ_phase2.py

Same question here, and I assume for the other python files in this set.

aloeliger · 2024-01-17T16:58:22Z

L1Trigger/L1TMuonOverlapPhase2/interface/InputMakerPhase2.h

+  DtPhase2DigiToStubsConverterOmtf(const OMTFConfiguration* config,
+                                   const OmtfAngleConverter* angleConverter,


This doesn't seem possible due to the amount of underlying code that needs to use these (so don't worry about this), but I don't suppose either of these could be references, and not have to worry about having nullptrs around?

epalencia · 2024-02-02T12:57:29Z

@mbluj , could you please address Andrew's comments?

mbluj · 2024-02-02T13:51:01Z

@mbluj , could you please address Andrew's comments?

Yes, sorry for this delay. I was absent and then overflooded by other commitments. I plan to back to it next week.

aloeliger · 2024-02-13T12:57:09Z

Hi @mbluj, I'm going around starting to figure out how to update all PRs to our new IB

For this PR I would recommend:

First, make a backup of this branch and it's commits pushed to github under a different name
checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
Get a copy of this branch locally via git cms-checkout-topic -u mbluj:phase2-l1t-integration-13_3_0_pre3_mbluj_OMTF_ph2
Perform an interactive rebase via git rebase from-CMSSW_14_0_0_pre3 --interactive
In the interactive rebase, please drop all commits that were not originally part of this pull request. In my tests, there seemed to be some earlier commits from track trigger commits that get caught up in this somehow?
You can either then push the resulting branch to the current PR branch, or make a new one and make a PR from that branch to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3

Alternatively, because you have relatively few commits, you could just cherry-pick commits by doing something like:

First, make a backup of this branch and it's commits pushed to github under a different name
checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
Fetch your changes git fetch my-cmssw
Cherry pick the 4 necessary commits via git cherry-pick, the 4 commands needed (in order) I think are: git cherry-pick 20dacb5c0e47d54008bacd8e08d9cd52b40622bd git cherry-pick 3323c2e6914f707a6837977c5cc7172a1baeda9b and git cherry-pick 283fc1d619f86e57efa53e3437f379dc09be6f13
You can either then push the resulting branch to the current PR branch, or make a new one and make a PR from that branch to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3

mbluj · 2024-02-13T13:36:18Z

OK, I will rebase it to newer release, I think I know how do it.
Do I understand correctly that you want it be based on plain CMSSW_14_0_0_pre3? Will not it be better to rebase to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3? But, both are fine with me.

zhenbinwu · 2024-02-15T17:36:04Z

OK, I will rebase it to newer release, I think I know how do it. Do I understand correctly that you want it be based on plain CMSSW_14_0_0_pre3? Will not it be better to rebase to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3? But, both are fine with me.

In order to get both phase-2 EMTF and OMTF to the GMT, I would prefer a rebase to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3, since EMTF is merged. Thanks

mbluj · 2024-02-16T13:02:58Z

@aloeliger I rebased my development branch to phase2-l1t-integration-14_0_0_pre3 and changed target base to it.

triggerDoctor · 2024-02-16T14:05:56Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-02-16T14:16:46Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-02-16T14:17:11Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

mbluj · 2024-02-16T14:49:02Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc

Please run scram b code-format to auto-apply code formatting

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-format-all

This file is not modified by this PR. And I do not see issue with its formatting.

mbluj · 2024-02-21T17:48:31Z

PR updated with cherry-picked cleanup of OMTF phase1 code and with cleanup of OMTF phase2 code. There are, however, still a few items to address.

triggerDoctor · 2024-02-24T09:59:45Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-02-24T10:12:10Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-02-24T10:12:30Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

aloeliger · 2024-02-26T12:14:33Z

@mbluj Are there any remaining review comments to be addressed here? I think we would like to merge this draft, and have a PR made for any phase 2 elements that have not already had a PR to central CMSSW. @epalencia any objection to your side towards beginning to merge this into the prototype?

mbluj · 2024-02-26T12:21:08Z

@mbluj Are there any remaining review comments to be addressed here? I think we would like to merge this draft, and have a PR made for any phase 2 elements that have not already had a PR to central CMSSW. @epalencia any objection to your side towards beginning to merge this into the prototype?

I will commit today a few additional fixes from Karol. So, please hold on until it is done, please.

mbluj · 2024-02-26T13:15:59Z

Added cleanups from Karol:

removal of a few expert configurations (cfg files),
replacement of const ptrs by const refs in DtPhase2DigiToStubsConverter.

As the changes are small and not specific were squashed with the previous commit with cleanups.

Last missing piece is fixing propagation in CandidateSimMuonMatcher.cc which should not depend on point where a propagated muon is produced. It is basically ready and can be added this afternoon (here and to phase1 PR)

mbluj · 2024-02-26T13:42:33Z

@aloeliger @epalencia, commit with updated propagation in CandidateSimMuonMatcher.cc is added and now we consider updates of this PR compete.

triggerDoctor · 2024-02-26T16:45:45Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-02-26T16:53:18Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-02-26T16:53:38Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 2 files that did not meet formatting requirements:

L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc
L1Trigger/L1TMuonOverlapPhase2/interface/InputMakerPhase2.h

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

epalencia · 2024-02-26T16:57:19Z

@mbluj , can you check the format issue in L1Trigger/L1TMuonOverlapPhase2/interface/InputMakerPhase2.h?

mbluj · 2024-02-27T11:07:29Z

@mbluj , can you check the format issue in L1Trigger/L1TMuonOverlapPhase2/interface/InputMakerPhase2.h?

OK. There is indeed small formatting issue. I am going to fix it and squash with earlier commit to not add new trivial items to history.

mbluj · 2024-02-27T11:08:55Z

@epalencia Corrected branch pushed.

aloeliger · 2024-02-28T10:47:22Z

@epalencia Any objection to merging this now? The phase 1 PR has been signed in CMSSW already.

epalencia · 2024-02-28T11:16:29Z

No. Please go ahead.

triggerDoctor · 2024-02-28T13:41:51Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -j 8`

triggerDoctor · 2024-02-28T13:53:03Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks`

I found no issues with the headers!

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 check-headers`

triggerDoctor · 2024-02-28T13:53:33Z

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

L1Trigger/L1TMuon/src/MicroGMTConfiguration.cc

Please run scram b code-format to auto-apply code formatting

Info	Value
return code	`0`
command	`eval` scramv1 runtime -sh `&& scram b -k -j 8 code-format-all`

aloeliger · 2024-02-28T15:01:05Z

@mbluj To provide the menu team with validation material, we are going to go ahead and merge this branch into the integration branch. If there are any elements of this (Phase 2 or otherwise) that haven't been put in a PR to central CMSSW, please make a PR for that now. @epalencia FYI.

mbluj · 2024-02-28T15:04:03Z

@mbluj To provide the menu team with validation material, we are going to go ahead and merge this branch into the integration branch. If there are any elements of this (Phase 2 or otherwise) that haven't been put in a PR to central CMSSW, please make a PR for that now. @epalencia FYI.

Thanks, great! FYI, @kbunkow

mbluj force-pushed the phase2-l1t-integration-13_3_0_pre3_mbluj_OMTF_ph2 branch from 8bc105a to 283fc1d Compare January 11, 2024 11:16

aloeliger reviewed Jan 17, 2024

View reviewed changes

aloeliger added Phase-1 Pertains to phase-1 development Phase-2 Pertains to phase-2 development Emulator Development Emulator development PR Physics Affecting A PR expected to affect Physics content of the trigger labels Jan 19, 2024

aloeliger changed the base branch from phase2-l1t-integration-13_3_0_pre3 to phase2-l1t-integration-14_0_0_pre3 February 5, 2024 15:43

aloeliger added the Needs Rebase PR should be rebased to newer branch label Feb 13, 2024

aloeliger changed the base branch from phase2-l1t-integration-14_0_0_pre3 to phase2-l1t-integration-13_3_0_pre3 February 13, 2024 12:54

akalinow mentioned this pull request Feb 14, 2024

XML cofing files for the OMTF displaced algo. cms-data/L1Trigger-L1TMuon#26

Merged

mbluj added 3 commits February 16, 2024 13:45

Refactoring and cleaning of Phase1 OMTF emulator code

c9c91ad

OMTF emulator for phase2

589b827

code-checks

9e1e558

mbluj force-pushed the phase2-l1t-integration-13_3_0_pre3_mbluj_OMTF_ph2 branch from 283fc1d to 9e1e558 Compare February 16, 2024 12:55

mbluj changed the base branch from phase2-l1t-integration-13_3_0_pre3 to phase2-l1t-integration-14_0_0_pre3 February 16, 2024 13:00

mbluj force-pushed the phase2-l1t-integration-13_3_0_pre3_mbluj_OMTF_ph2 branch from 84a5e52 to 758c7a5 Compare February 26, 2024 12:48

mbluj added 2 commits February 27, 2024 12:05

cleanup in L1Trigger/L1TMuonOverlapPhase2

5b7f24b

Update propagation to station 2

531046d

mbluj force-pushed the phase2-l1t-integration-13_3_0_pre3_mbluj_OMTF_ph2 branch from 52ce8d5 to 531046d Compare February 27, 2024 11:08

aloeliger merged commit 77fa72a into cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3 Feb 28, 2024

zhenbinwu mentioned this pull request Mar 1, 2024

Update of the GMT Emulator #1203

Merged

zhenbinwu mentioned this pull request Mar 22, 2024

Update of Phase-2 GMT Emulator cms-sw/cmssw#44498

Merged

mbluj mentioned this pull request Mar 27, 2024

Phase2 OMTF emulator cms-sw/cmssw#44557

Merged

folguera mentioned this pull request Apr 11, 2024

OMTF configuration fix to use phase-2 values #1247

Merged

		DtPhase2DigiToStubsConverterOmtf(const OMTFConfiguration* config,
		const OmtfAngleConverter* angleConverter,

Refactoring of Phase1 OMTF emulator & Phase2 OMTF emulator #1196

Refactoring of Phase1 OMTF emulator & Phase2 OMTF emulator #1196

Conversation

mbluj commented Jan 10, 2024

PR description:

PR validation:

triggerDoctor commented Jan 11, 2024

triggerDoctor commented Jan 11, 2024

triggerDoctor commented Jan 11, 2024

mbluj commented Jan 11, 2024

mbluj commented Jan 11, 2024 • edited Loading

aloeliger left a comment

Choose a reason for hiding this comment

aloeliger Jan 17, 2024

Choose a reason for hiding this comment

mbluj Feb 21, 2024

Choose a reason for hiding this comment

aloeliger Jan 17, 2024

Choose a reason for hiding this comment

mbluj Feb 21, 2024

Choose a reason for hiding this comment

aloeliger Jan 17, 2024

Choose a reason for hiding this comment

aloeliger Jan 17, 2024

Choose a reason for hiding this comment

epalencia commented Feb 2, 2024

mbluj commented Feb 2, 2024

aloeliger commented Feb 13, 2024

mbluj commented Feb 13, 2024

zhenbinwu commented Feb 15, 2024

mbluj commented Feb 16, 2024

triggerDoctor commented Feb 16, 2024

triggerDoctor commented Feb 16, 2024

triggerDoctor commented Feb 16, 2024

mbluj commented Feb 16, 2024

mbluj commented Feb 21, 2024 • edited Loading

triggerDoctor commented Feb 24, 2024

triggerDoctor commented Feb 24, 2024

triggerDoctor commented Feb 24, 2024

aloeliger commented Feb 26, 2024

mbluj commented Feb 26, 2024

mbluj commented Feb 26, 2024

mbluj commented Feb 26, 2024

triggerDoctor commented Feb 26, 2024

triggerDoctor commented Feb 26, 2024

triggerDoctor commented Feb 26, 2024

epalencia commented Feb 26, 2024

mbluj commented Feb 27, 2024

mbluj commented Feb 27, 2024

aloeliger commented Feb 28, 2024

epalencia commented Feb 28, 2024

triggerDoctor commented Feb 28, 2024

triggerDoctor commented Feb 28, 2024

triggerDoctor commented Feb 28, 2024

aloeliger commented Feb 28, 2024

mbluj commented Feb 28, 2024

mbluj commented Jan 11, 2024 •

edited

Loading

mbluj commented Feb 21, 2024 •

edited

Loading