Merge l1t-tsg-v5 into CMSSW

[mulhearn]

This issue covers the preparations to merge l1t-tsg-v5 into CMSSW.

Currently, the PRs bring us upto date with l1t-tsg-v34.0

The 81x Prs are:

• Bring L1T (nearly) too l1t-tsg-v5 (81x version) cms-sw/cmssw#14104 the main pr (merged)
• L1T Ntuple updates for l1t-tsg-v5 cms-sw/cmssw#14028 updates to ntuples.. not critical. (needs comments addressed)
• Bring L1T up-to-date with l1t-tsg-v5-cand cms-sw/cmssw#14137 incremental update to config as in l1t-tsg-v5-cand (merged)
• IB crash fix Fix IB crashes due to BX range set improperly cms-sw/cmssw#14245 (passes tests, fully signed)

The 80x Prs are:

• Bring L1T up-to-date with l1t-tsg-v5-cand (80x) cms-sw/cmssw#14141 80x version of main-pr plus incremental updates (merged)
• (ntuple PR will be made after issues in 81x are addressed)
• Pr 14141 plus bxvector fix 80x cms-sw/cmssw#14246 (testing)

The external data requests are:

• Additional 2 Tau LUTs needed for l1t-tsg-v5 merge cms-data/L1Trigger-L1TCalorimeter#10 (merged)

NOTE: Will need a small increment to add configuration change of caloParams to use Tau Iso LUTs option-5 (i.e. l1t-tsg-v5.1)

[mulhearn]

Data area PRs are here:
cms-data/L1Trigger-L1TCalorimeter#9
cms-data/L1Trigger-L1TGlobal#3

[mulhearn]

Stand-in PR into 80x is here: cms-sw#14024
This will be updated once the tsg-v4 PRs are merged.

[nsmith-]

I really really really think this is a bad idea to rewrite the history.

• When someone asks 'did my commit get into this release?' they can't just do git branch --contains but have to read the code.
• When someone asks 'who wrote this line? Can I contact them about it?' the answer is unavailable.
• When someone tries to merge their development branch that was split off from l1t-integration at any time since tsg-v4, up to a new CMSSW version, now they can't.
• Everyone in cms-l1t-offline will have to halt development and move their private development branches to this new changeset to have any hope of having an easy PR to integration

This is all because people don't want too many commits in the git history? I'm sure the servers aren't complaining, and git is happy to have plenty of commits. In particular, merge commits are very lightweight objects if not changing anything, which is the case whenever there is no merge conflict.

[fwyzard]

Rewriting history can be good or bad, it depends how you do it. I agree that simply squashing the whole set of changes that came after l1t-tsg-v4 into a single commit is a bad idea. Squashing each set of commits into one or two could definitely increase the readability of the changes.

For example, now when someone asks "what changed between these two tags" he sees a bunch commits with messages like

29281f1 Keep comments consistent
5dfdce0 oops
b4256ab Lots of things, losing track
2fe4425 Formatting

which clearly are not particularly useful.

Personally, I would be in favour of structuring the pull requests along the philosophy used by the patches for the linux kernel, https://www.kernel.org/doc/Documentation/SubmittingPatches, section 3:

Separate each logical change into a separate patch.

For example, if your changes include both bug fixes and performance
enhancements for a single driver, separate those changes into two
or more patches. If your changes include an API update, and a new
driver which uses that new API, separate those into two patches.

On the other hand, if you make a single change to numerous files,
group those changes into a single patch. Thus a single logical change
is contained within a single patch.

The point to remember is that each patch should make an easily understood
change that can be verified by reviewers. Each patch should be justifiable
on its own merits.

If one patch depends on another patch in order for a change to be
complete, that is OK. Simply note "this patch depends on patch X"
in your patch description.

When dividing your change into a series of patches, take special care to
ensure that the kernel builds and runs properly after each patch in the
series. Developers using "git bisect" to track down a problem can end up
splitting your patch series at any point; they will not thank you if you
introduce bugs in the middle.

[nsmith-]

Ahh you found a few gems I should have squashed. : <
I hope my personal deficiencies don't distract from the message here...

In principle we can cut the number of merge commits down to 1 by this method of development:

git checkout -b $USER-dev-$CMSSW_VERSION cms-l1t-offline/l1t-integration-$CMSSW_VERSION
git cms-sparse-checkout $CMSSW_VERSION HEAD
git read-tree -mu HEAD
scram b
# hack...
git push cms-l1t-offline $USER-dev-$CMSSW_VERSION

and then when merging the PR, just allow github to do it with one merge commit from feature to master.

For fun I made this diagram of what's happened since l1-tsg-v4:
https://ncsmith.web.cern.ch/ncsmith/L1TStage2Misc/graph.html
(I don't think its that convoluted, really)

[mulhearn]

Hi Nate, I understand and share your concerns but in this case it is the lesser of two evils. Bear in mind:

1. We are still in rapid development mode. Eventually, we will move to a mode where PRs are reviewed, commits are quashed, and integration tests are run, all at time of the PR. Ideally, I'd like to move to a mode where our integration tests have been run at every commit. But the developers are overloaded right now, so I am handling as much of the git manipulations as I can for now. It would just frankly cause too much delay to preserve existing commits and have viable small PRs.

2. We have a clear record of what made each l1t-integration-vX tag, and these squashed commits all reference those. So there's no need to read code to know exactly what commits made these PRs.

3. Note that it is logically inconsistent to want no history cleanup and complain about graph complexity in the same post! You can pick a clean history or an honest history, but not both, except in purely contrived examples.

There are pluses and minus to each of the various approaches, and in this case, I think this one is optimal choice.

[mulhearn]

4. We are forward and back porting between 80X and 81X depending on the context.

[mulhearn]

I invite any interested parties, as an exercise, to take tag l1t-tsg-v5 and break it into 3-4 smaller PRs without changing any commit history. And don't forget not to include the data area in any commit that goes to CMSSW while you are at it! Time how long it takes you. I've done such an experiment with earlier tags... it is definitely possible (except for the data area) but it takes forever, for very marginal gains. I'm open to suggestions, honestly, but I've experimented quite heavily and am choosing what seems optimal to me.

[fwyzard]

I actually tried to do it for l1t-tsg-v4, and eventually failed to even get the same final code :-(

[mulhearn]

These are now obsolete, removing from summary and recording here:

83bf154174b48d5426b834121342c50cd3ce8907 L1T data updates for l1t-tsg-v5, for testing until in externals.
30b7e496f648314df117e673db93c99cbde96134 L1TNtuples updates for l1t-tsg-v5
91b45e068610694d5a0d58fa1d3dc9142bebffa6 L1T algorithm updates for l1t-tsg-v5
60d11875290c7178f4d89bed29513d7006612bdb unpacker updates for l1t-tsg-v5
bad29cb552c3d5596b0e3b41efa0b408b23f1318 L1T data formats updates for l1t-tsg-v5
24e7173019f3be4736586e32b3d8018c66f3d809 CondFormats updates for l1t-tsg-v5
5edb1c418d8c1bf53dc948556dcd5e9e5fef4a79 Merged refs/pull/14021/head from repository cms-sw

[mulhearn]

I've been doing some bitwise comparisons of l1t-tsg-v5 to the above PRs.

I prepare l1t-tsg-v5 according to the standard recipe plus:

• use MC tag (as in 804): 80X_mcRun2_asymptotic_v9
• use Menu (as in 804 GT): L1Menu_Collisions2015_25nsStage1_v7_uGT_v4.xml

Then I compare with the 80X PR after:

• git revert -m 1 abd4163622272f195b1e1ee533acd02212fe6a28
• git revert -m 1 17ab3e96ef8139dc14098336f20c7f74f4ca5a39
• git cherry-pick 83bf154174b48d5426b834121342c50cd3ce8907
  in order to revert the HF 1x1 LUT changes that effect results, and get the data files that haven't been added as external in release yet.

This gives me bitwise agreement for mc production and re-emulation, except for the Menu, which differs by the External Conditions (which aren't yet re-emulated in 80x):

<        0                                L1_ZeroBias         0      0      0         1          1        0
<        1                       L1_SingleEG2_BptxAND         0      0      0         1          1        0
---
>        0                                L1_ZeroBias       100    100    100         1          1        0
>        1                       L1_SingleEG2_BptxAND        98     98     98         1          1        0
11c11
<        4                           L1_ETT15_BptxAND         0      0      0         1          1        0
---
>        4                           L1_ETT15_BptxAND       100    100    100         1          1        0
69c69
<       62                  L1_SingleJetC32_NotBptxOR        99     99     99         1          1        0
---
>       62                  L1_SingleJetC32_NotBptxOR         0      0      0         1          1        0
79c79
<       72                  L1_SingleMuOpen_NotBptxOR        99     99     99         1          1        0
---
>       72                  L1_SingleMuOpen_NotBptxOR         0      0      0         1          1        0
95c95
<       88                  L1_SingleJetC20_NotBptxOR       100    100    100         1          1        0
---
>       88                  L1_SingleJetC20_NotBptxOR         0      0      0         1          1        0
149,156c149,156
<      142                L1_DoubleMu0_Eta1p6_WdEta18         0      0      0         1          1        0
<      143             L1_DoubleMu0_Eta1p6_WdEta18_OS         0      0      0         1          1        0
<      144                   L1_DoubleMu_10_0_WdEta18         0      0      0         1          1        0
<      145          L1_IsoEG20er_TauJet20er_NotWdEta0         0      0      0         1          1        0
<      146   L1_Jet32_DoubleMu_Open_10_MuMuNotWdPhi23_JetMuWdPhi1         0      0      0         1          1        0
<      147   L1_Jet32_MuOpen_EG10_MuEGNotWdPhi3_JetMuWdPhi1         0      0      0         1          1        0
<      148                    L1_Mu3_JetC16_WdEtaPhi2         0      0      0         1          1        0
<      149                    L1_Mu3_JetC52_WdEtaPhi2         0      0      0         1          1        0
---
>      142                L1_DoubleMu0_Eta1p6_WdEta18       100    100    100         1          1        0
>      143             L1_DoubleMu0_Eta1p6_WdEta18_OS       100    100    100         1          1        0
>      144                   L1_DoubleMu_10_0_WdEta18       100    100    100         1          1        0
>      145          L1_IsoEG20er_TauJet20er_NotWdEta0       100    100    100         1          1        0
>      146   L1_Jet32_DoubleMu_Open_10_MuMuNotWdPhi23_JetMuWdPhi1       100    100    100         1          1        0
>      147   L1_Jet32_MuOpen_EG10_MuEGNotWdPhi3_JetMuWdPhi1       100    100    100         1          1        0
>      148                    L1_Mu3_JetC16_WdEtaPhi2       100    100    100         1          1        0
>      149                    L1_Mu3_JetC52_WdEtaPhi2       100    100    100         1          1        0

So this all looks good. The only ones I'm not sure of are those last ones, which don't explicitly list an external...

[tmatsush]

Algorithms with correlation conditions (WdEta, WdPhi etc) in this menu are place holders as explained at
https://twiki.cern.ch/twiki/bin/view/CMS/GlobalTriggerMenu_L1Menu_Collisions2015_25nsStage1_v7_uGT_v4#Introduction

The conditions used for these algorithms are EXT_BPTX_plus_AND_minus.v0 as can be seen at
https://twiki.cern.ch/twiki/pub/CMS/GlobalTriggerMenu_L1Menu_Collisions2015_25nsStage1_v7_uGT_v4/L1Menu_Collisions2015_25nsStage1_v7_uGT_v4.html

[mulhearn]

OK, thanks, Takashi.

[mulhearn]

Also compared the 80x PR and the 81x PR. They differ only by trigger results from externals (which are emulated in 81x but not yet in 80x)

[mulhearn]

As additional check, in 80X PR:

mulhearn@cmsdev02>git diff --name-only $CMSSW_VERSION | xargs git diff --name-only l1t-integration-v34.0 --
EventFilter/L1TXRawToDigi/python/util/.gitignore
L1Trigger/L1TCaloLayer1/plugins/L1TCaloLayer1.cc
L1Trigger/L1TCaloLayer1/src/L1TCaloLayer1FetchLUTs.cc
L1Trigger/L1TCaloLayer1/src/UCTRegion.cc
L1Trigger/L1TCaloLayer1/src/UCTTower.cc
L1Trigger/L1TGlobal/test/runGlobalFakeInputProducer.py

Likewise for 81x PR:

mulhearn@cmsdev03>git diff --name-only $CMSSW_VERSION | xargs git diff --name-only l1t-integration-v34.0 --
EventFilter/L1TXRawToDigi/plugins/L1TCaloLayer1RawToDigi.cc
EventFilter/L1TXRawToDigi/python/util/.gitignore
L1Trigger/L1TCaloLayer1/plugins/L1TCaloLayer1.cc
L1Trigger/L1TCaloLayer1/src/L1TCaloLayer1FetchLUTs.cc
L1Trigger/L1TCaloLayer1/src/UCTRegion.cc
L1Trigger/L1TCaloLayer1/src/UCTTower.cc
L1Trigger/L1TCalorimeter/plugins/L1TCaloParamsESProducer.cc
L1Trigger/L1TCalorimeter/plugins/L1TStage2Layer2Producer.cc
L1Trigger/L1TMuon/plugins/L1TMuonGlobalParamsESProducer.cc

And checked that these discrepancies are all expected...

[mulhearn]

cms-sw#14104 is ready for merge.

[mulhearn]

Clint stopped by to say he had seem some discrepancies with his rate studies using HLT recipe in 805+PR 14141 and Zhenbin's studies (802 + l1t-tsg-v5).

I ran full statistics bitwise comparisons and had perfect agreements between 805+PR 14141 and l1t-tsg-v5 (in 802 as in recipe) plus:

• git cherry-pick -m 1 17ab3e96ef8139dc14098336f20c7f74f4ca5a39
• use MC tag (as in 805): 80X_mcRun2_asymptotic_v9
• use Menu (as in 805 GT): L1Menu_Collisions2015_25nsStage1_v7_uGT_v4.xml

I think the discrepancies must therefore be between the different analysis software of Zhenbin and Clint.

One remaining good comparison tests would be to check vanilla 810_pre3 against 805+PR14141 and nothing else...

[fwyzard]

On 26 April 2016 at 01:58, mulhearn [email protected] wrote:

I think the discrepancies must therefore be between the different analysis
software of Zhenbin and Clint.

Clint is using the uGt emulator, while Zhenbin is simulating the L1
decision based on an independent C++ implementation of the algorithms.

Len and Zhenbin have already reported some discrepancies from a similar
comparison, for example in the VBF triple jet triggers.

This does not mean that there is anything wrong with l1t-tsg-v5 (apart from
the known issues).

.A

[zhenbinwu]

@fwyzard The difference between uGT emulator and C++ implementation mostly exists for the correlation conditions for now. The VBF jet are fixed.

Clint saw a large discrepancy with my singleMu rate. I got 10kHz while he got 5kHz ( similar to the TSGv4 value). I suspected he is using the old TSGv4 tag. But it seems not the case...