Bring L1T emulation up to date with l1t-tsg-v4 tag #13767

mulhearn · 2016-03-18T02:50:25Z

Brings the L1T emulator algorithms up to date with the l1t-tsg-v4 tag.

This depends on data area PRs:

…ript - thanks Nick Smith

Conflicts: L1Trigger/L1TCalorimeter/data/Flat_Tau_iso_LUT_eff70.txt L1Trigger/L1TCalorimeter/data/Flat_Tau_iso_LUT_eff80.txt L1Trigger/L1TCalorimeter/data/Flat_Tau_iso_LUT_eff90.txt

… the raw track address.

…muons if no muon was in the BX.

* the phi angle converions follows hardware mroe closely

perrotta · 2016-04-05T10:33:07Z

DataFormats/L1TMuon/interface/EMTFOutput.h

@@ -70,7 +71,8 @@ namespace l1t {
    emtf::EventTrailer GetEventTrailer()    { return EventTrailer;  };
    emtf::MTF7Trailer GetMTF7Trailer()      { return MTF7Trailer;   };
    emtf::AMC13Trailer GetAMC13Trailer()    { return AMC13Trailer;  };
-    const uint64_t Dataword() const     { return dataword; };
+    const int Format_Errors() const { return format_errors; };
+    const uint64_t Dataword() const { return dataword; };


@mulhearn
These two methods (Format_Errors() and Dataword()) are both missing in the 81X version of this file (#13768), while all methods from line 54 to line 73 were const in the 81X incarnation, but not here. Is all that wanted, or any of the two PR's for 81X or 80X should be fixed, instead?

Right, yeah, that was one of the conflicted files when merging this PR into 81X. I guess some DQM changes were already merged in 81X. I'm pretty sure now this is a harmless difference, but I am still checking.

It was the DQM that made these changes:

mulhearn@lxplus071>git show aeba2b41b2de9ec4382915a4ba29257e410c7800 -- DataFormats/L1TMuon/interface/EMTFOutput.h commit aeba2b41b2de9ec4382915a4ba29257e410c7800 Author: Sean-Jiun Wang <churchguitarist@gmail.com> Date: Mon Mar 7 18:04:48 2016 +0100 First commit: compiles and runs diff --git a/DataFormats/L1TMuon/interface/EMTFOutput.h b/DataFormats/L1TMuon/interface/EMTFOutput.h index 7d607b3..bc8968d 100644 --- a/DataFormats/L1TMuon/interface/EMTFOutput.h +++ b/DataFormats/L1TMuon/interface/EMTFOutput.h @@ -50,27 +50,27 @@ namespace l1t { void set_AMC13Trailer(emtf::AMC13Trailer bits) { AMC13Trailer = bits; hasAMC13Trailer = true; }; void set_dataword(uint64_t bits) { dataword = bits; }; - bool HasAMC13Header() { return hasAMC13Header; }; - bool HasMTF7Header() { return hasMTF7Header; }; - bool HasEventHeader() { return hasEventHeader; }; - bool HasCounters() { return hasCounters; }; - int NumSP() { return numSP; }; - int NumRPC() { return numRPC; }; - int NumME() { return numME; }; - bool HasAMC13Trailer() { return hasAMC13Trailer; }; - bool HasMTF7Trailer() { return hasMTF7Trailer; }; - bool HasEventTrailer() { return hasEventTrailer; }; - emtf::AMC13Header GetAMC13Header() { return AMC13Header; }; - emtf::MTF7Header GetMTF7Header() { return MTF7Header; }; - emtf::EventHeader GetEventHeader() { return EventHeader; }; - emtf::Counters GetCounters() { return Counters; }; - emtf::MECollection GetMECollection() { return MECollection; }; - emtf::RPCCollection GetRPCCollection() { return RPCCollection; }; - emtf::SPCollection GetSPCollection() { return SPCollection; }; - emtf::EventTrailer GetEventTrailer() { return EventTrailer; }; - emtf::MTF7Trailer GetMTF7Trailer() { return MTF7Trailer; }; - emtf::AMC13Trailer GetAMC13Trailer() { return AMC13Trailer; }; - const uint64_t Dataword() const { return dataword; }; + const bool HasAMC13Header() const { return hasAMC13Header; }; + const bool HasMTF7Header() const { return hasMTF7Header; }; + const bool HasEventHeader() const { return hasEventHeader; }; + const bool HasCounters() const { return hasCounters; }; + const int NumSP() const { return numSP; }; + const int NumRPC() const { return numRPC; }; + const int NumME() const { return numME; }; + const bool HasAMC13Trailer() const { return hasAMC13Trailer; }; + const bool HasMTF7Trailer() const { return hasMTF7Trailer; }; + const bool HasEventTrailer() const { return hasEventTrailer; }; ...

Why do you even have separate branches for the code for 8.0.x and 8.1.x ?

At the very least, they list of commits should be the same, cherry-picked form one release (whatever you consider the one people should be developing with) to the other.

Uggh, this was a pain to track down... this difference is due to the DQM PR (#13891):

mulhearn@lxplus071>git show --name-only aeba2b41b2de9ec4382915a4ba29257e410c7800 commit aeba2b41b2de9ec4382915a4ba29257e410c7800 Author: Sean-Jiun Wang <churchguitarist@gmail.com> Date: Mon Mar 7 18:04:48 2016 +0100 First commit: compiles and runs DQM/Integration/python/clients/l1tstage2_dqm_sourceclient-live_cfg.py DQM/L1TMonitor/interface/L1TStage2EMTF.h DQM/L1TMonitor/python/L1TStage2EMTF_cfi.py DQM/L1TMonitor/python/L1TStage2_cff.py DQM/L1TMonitor/src/L1TStage2EMTF.cc DQM/L1TMonitor/src/SealModule.cc DataFormats/L1TMuon/interface/EMTFOutput.h EventFilter/L1TRawToDigi/python/emtfStage2Digis_cfi.py

this will all be consistent again once the PR is merged, and Vladimir makes the 80x version of that DQM PR.

@fwyzard: because there were merge conflicts when merging 80x version of PR into 81x.

why?
I would hope the L1 code is not diverging between 80x and 81x

On Apr 6, 2016, at 1:38 PM, mulhearn notifications@github.com wrote:

In DataFormats/L1TMuon/interface/EMTFOutput.h:

@@ -70,7 +71,8 @@ namespace l1t {
emtf::EventTrailer GetEventTrailer() { return EventTrailer; };
emtf::MTF7Trailer GetMTF7Trailer() { return MTF7Trailer; };
emtf::AMC13Trailer GetAMC13Trailer() { return AMC13Trailer; };

const uint64_t Dataword() const { return dataword; };

const int Format_Errors() const { return format_errors; };

const uint64_t Dataword() const { return dataword; };

@fwyzard: because there were merge conflicts when merging 80x version of PR into 81x.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

The point here is the code in 80X is diverging from 81X on account of a separate PR as Mike mentioned. (#13891)
If we were to compare the L1 packages at a point in the 81X history before this other PR, I suspect no discrepancy would be found.

cmsbuild · 2016-04-05T11:02:24Z

+1
Tested at: 0086089
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13767/12172/summary.html

cmsbuild · 2016-04-05T11:44:19Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13767/12172/summary.html

mulhearn · 2016-04-06T10:26:26Z

+1

mulhearn · 2016-04-06T10:27:21Z

Following Perrotta's comments I did a check of all L1T files between this and the 81X version of this PR. All differences are understood.

mulhearn · 2016-04-06T10:45:49Z

+1

davidlt · 2016-04-06T10:56:56Z

Are you really changing 168 files with 239 commits, or the history here is trashed with merge commits?

mulhearn · 2016-04-06T11:40:06Z

Well, not quite trashed, e.g. I don't see a bunch of duplicate version of same commit, but the usual multiple useless empty merge commit business plus lots of small commits.

davidlt · 2016-04-06T11:49:47Z

Why did you had to do git merge? How many of these commits out of 239 should actually be part of this PR? Just trying to understand.

nsmith- · 2016-04-06T11:56:37Z

The typical workflow for a change in L1 trigger land is something like:

cmsrel ....
...
git cms-merge-topic cms-l1t-offline:l1t-integration-...
... edit
git push cms-l1t-offline my-change-branch

So every time someone makes a change and makes PR to l1t-integration, there is the empty merge commit that brings the HEAD (from-CMSSW...) up to where cms-l1t-offline is at.

This is where most of the merge commits come from, they are harmless, and just an artifact of the development process that L1 offline has subscribed to.

Granted, there are alternatives that might make the commit history cleaner, but they require commands more unfamiliar to cmssw users

mulhearn · 2016-04-06T12:00:47Z

Yes, exactly. I'm considering cleaning up commit history at point we push into official CMSSW in future, but definitely not here in this PR.

mulhearn · 2016-04-06T12:04:28Z

And adding to nsmith, you also get two more merge commits if you follow github standard recipe for merging their PRs into the integration branch. So one small change can have four commits.

mulhearn · 2016-04-06T12:06:20Z

At integration branch level, we need to keep the developer commits intact so we can sort out what happens. But once we make a tag (l1t-tsg-vX), I think it would be fine to push to CMSSW a cleaned up history with fewer topical commits ("bring L1TGlobal up-to date with l1t-tsg-vX").

mulhearn · 2016-04-06T12:07:25Z

But that robs developers of their credit, which is actually a non-neglible consideration now that some firms check github before hiring...

mulhearn · 2016-04-06T12:08:31Z

Anyway, let's merge this sucka.

davidlange6 · 2016-04-06T12:11:15Z

Let's fix all the 81x problems first;)

Cheers,
David

On 06 Apr 2016, at 14:08, mulhearn <notifications@github.com mailto:notifications@github.com> wrote:

Anyway, let's merge this sucka.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/13767#issuecomment-206342564

fwyzard · 2016-04-06T15:55:50Z

On 6 April 2016 at 14:07, mulhearn notifications@github.com wrote:

But that robs developers of their credit, which is actually a non-neglible
consideration now that some firms check github before hiring...

In general I agree, I prefer to leave the commit to original authors - if
anything to know who to blame for the bugs and credit for the fixes ;-)

You can rebase most of the merges away, and also squash successive commits,
without changing the author.

But trying to rebase this PR now is simply impossible - and pointless,
since we have the equivalent one in 8.1.x already.

.A

mmusich · 2016-04-12T08:16:44Z

+1

mulhearn and others added 30 commits February 9, 2016 12:14

update L1T standard sequences

5406f87

cleanup summary module and related customs

d8e4835

add ReEmul customization for cmsDriver workflows and more utilities

b0824f2

Update EG isolation + Tau calibration LUTs

7dcb5dc

Fixed HBHENoiseFilterResult

3d742d9

new emulator for calo layer1

8fc7039

unpacker for calo layer1

c168773

Added HF 1x1 handling

dae3208

Added HF tower handling

e12adb7

Fixed so HF TPs are appended to regular collection with version set to 1

0be1a05

Removed empty hfTPGDigis stub :(

9734d82

Added access to uctPhi() from tower for correct CaloTower making

4b1cedf

Added run number and dataset selection ability at run time to test sc…

91b0a53

…ript - thanks Nick Smith

move test code out of deprecated L1TCaloLayer1RawToDigi

6bfc69b

remove deprecated EventFilter/L1TCaloLayer1RawToDigi

2bd812a

fix l1t text summary in example code

2865eac

make L1TCaloLayer1 a drop in replacement, leave off by default

3489e9e

Added Options 4 and 5 isolation LUTs for taus

5897e1b

Conflicts: L1Trigger/L1TCalorimeter/data/Flat_Tau_iso_LUT_eff70.txt L1Trigger/L1TCalorimeter/data/Flat_Tau_iso_LUT_eff80.txt L1Trigger/L1TCalorimeter/data/Flat_Tau_iso_LUT_eff90.txt

quiet verbose reEmul example

fe177ee

Added absolute value operator to z-coordinate variable

faf8e0a

disable GMT regional muon packing until fixed

df59949

bugfix for standard sequences for pre6

0aa725f

remove unsupported examples

d5352f6

bugfix summary output and remove obsolete customs

3f8977c

add muons to emulation ntuple

2e2000a

re-emulate HCAL TPs by default in reEmul custom

e95c1eb

remove inadvertent override of HCAL TP re-emulation

4f28e8c

Check the size of the track address map before using it when builting…

74785f1

… the raw track address.

Bugfix: Every BX, not just the first one, is now filled with 6 empty …

66fd6f6

…muons if no muon was in the BX.

* make get methods of L1TMuonOverlapParams to be const

0531a74

* the phi angle converions follows hardware mroe closely

cmsbuild added the tests-started label Apr 5, 2016

perrotta reviewed Apr 5, 2016
View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Apr 5, 2016

cmsbuild added comparison-available and removed comparison-pending labels Apr 5, 2016

cmsbuild added l1-approved and removed l1-pending labels Apr 6, 2016

mulhearn mentioned this pull request Apr 11, 2016

Update of DQM combo PR for L1T, including updates from 81x #14021

Merged

cmsbuild added alca-approved and removed alca-pending labels Apr 12, 2016

davidlange6 merged commit b515c48 into cms-sw:CMSSW_8_0_X Apr 12, 2016

nsmith- mentioned this pull request Apr 19, 2016

L1T DQM-only PR #14149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring L1T emulation up to date with l1t-tsg-v4 tag #13767

Bring L1T emulation up to date with l1t-tsg-v4 tag #13767

mulhearn commented Mar 18, 2016

perrotta Apr 5, 2016

mulhearn Apr 6, 2016

mulhearn Apr 6, 2016

fwyzard Apr 6, 2016

mulhearn Apr 6, 2016

mulhearn Apr 6, 2016

davidlange6 Apr 6, 2016

nsmith- Apr 6, 2016

cmsbuild commented Apr 5, 2016

cmsbuild commented Apr 5, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

davidlt commented Apr 6, 2016

mulhearn commented Apr 6, 2016

davidlt commented Apr 6, 2016

nsmith- commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

davidlange6 commented Apr 6, 2016

fwyzard commented Apr 6, 2016

mmusich commented Apr 12, 2016

Bring L1T emulation up to date with l1t-tsg-v4 tag #13767

Bring L1T emulation up to date with l1t-tsg-v4 tag #13767

Conversation

mulhearn commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Apr 5, 2016

cmsbuild commented Apr 5, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

davidlt commented Apr 6, 2016

mulhearn commented Apr 6, 2016

davidlt commented Apr 6, 2016

nsmith- commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

mulhearn commented Apr 6, 2016

davidlange6 commented Apr 6, 2016

fwyzard commented Apr 6, 2016

mmusich commented Apr 12, 2016