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
EMTF BIG emulator update - attempt #1 #527
EMTF BIG emulator update - attempt #1 #527
Conversation
…configForTiltedTracker pr90x Configure TTStubAlgorithm from L1TrackTrigger sequence for TiltedTracker
update 2017 wf in IB: replace TenMuE with ZMM
Cleaning pixel certification to prevent crash at Tier0 [90X]
…CMSSW_9_0_0_pre4 Reject 0 pt Puppi candidates in jet-track association
Convert Validation/RecoTau modules from legacy to global producers
Resolve segfault in MuonGEMHitsHarvestor::dqmEndJob
…for-9_0 Ignoring calibration channels in the dataframes
HBHE M2 = fix Memory Leak (90X)
Various fixes: conddb core, conddb tools, runinfo o2o
@abrinke1 there are still a lot of other commits in the PR. |
@thomreis I'm aware of this: the changes were initially made on top of the l1t-integration-CMSSW_9_0_0 branch, and I spent the last day rebasing onto l1t-integration-CMSSW_9_1_0_pre3. You can see that there are ~300 files changed, of which > 250 are in the L1Trigger/L1TMuonEndCap/ area. These do not need to be tracked, since it's a total replacement. All the other changed files have been noted explicitly in my comments above. |
The DQM/L1TMonitor/src/L1TMP7ZeroSupp.cc and EventFilter/L1TRawToDigi/interface/Block.h src/Block.cc changes seem to have been lost in the transition to the 9_1_0_pre3 integration branch indeed. However, the corresponding PR cms-sw#17956 is now merged in the official CMSSW 91x. |
This is a huge number of files! |
Yep - hence the ALL CAPS :) |
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.
A few additional comments:
- I think the emulator should be in the l1t namespace.
- header files have the .hh ending compared to .h for most other packages. Is there a rule for this?
- How does the changed unpacker handle data from 2016? Does this work?
- I did not go through the new unpacker code in detail since I'm not familiar with the EMTF algorithm.
'Geometry/DTGeometryBuilder/data/dtSpecsFilter.xml', | ||
'Geometry/CSCGeometryBuilder/data/cscSpecsFilter.xml', | ||
'Geometry/CSCGeometryBuilder/data/cscSpecs.xml', | ||
'Geometry/RPCGeometryBuilder/data/PhaseII/RPCSpecs.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecs.xml', |
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.
Decrease in version number?
int is_CSC ; // 0 or 1. | ||
int is_RPC ; // 0 or 1. | ||
int is_GEM ; // 0 or 1. | ||
int subsystem ; // 1 or ?. 1 for CSC, 2 for RPC, 3 for GEM |
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.
A couple of question marks remain to be resolved.
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.
These comments are for documentation, I need to check them manually to be sure before removing question marks. Prefer to keep them as is for now, as a "to do" reminder.
phi_fp(-99), theta_fp(-99), phzvl(-99), ph_hit(-99), zone_hit(-99), zone_code(-99), | ||
fs_segment(-99), fs_zone_code(-99), bt_station(-99), bt_segment(-99), | ||
phi_loc(-99), phi_glob(-99), theta(-99), eta(-99), | ||
phi_sim(-99), theta_sim(-99), eta_sim(-99), |
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.
phi_glob(-99) and phi_sim(-99) are allowed values.
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.
Fixing.
rank(-99), winner(-99), charge(-99), bx(-99), first_bx(-99), second_bx(-99), | ||
pt(-99), pt_XML(-99), zone(-99), ph_num(-99), ph_q(-99), | ||
theta_fp(-99), theta(-99), eta(-99), phi_fp(-99), phi_loc(-99), phi_glob(-99), | ||
gmt_pt(-99), gmt_phi(-99), gmt_eta(-99), gmt_quality(-99), gmt_charge(-99), gmt_charge_valid(-99), |
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.
phi_glob(-99) is an allowed value. Check also gmt_phi(-99) and gmt_eta(-99)
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.
Fixing.
DataFormats/L1TMuon/src/EMTFHit.cc
Outdated
|
||
CSCDetId EMTFHit::CreateCSCDetId() const { | ||
return CSCDetId( (endcap == 1) ? 1 : 2, station, // For now, leave "layer" unfilled, defaults to 0. | ||
(ring == 4) ? 1 : ring, chamber ); // Not sure if this is correct, or what "layer" does. - AWB 27.04.16 |
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.
Is it correct?
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.
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.
Fixing
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecs.xml', | ||
'Geometry/GEMGeometryBuilder/data/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v5/GEMSpecs.xml', |
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.
Decreasing version number?
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecs.xml', | ||
'Geometry/GEMGeometryBuilder/data/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v5/GEMSpecs.xml', |
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.
Decreasing version number?
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecs.xml', | ||
'Geometry/GEMGeometryBuilder/data/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v5/GEMSpecs.xml', |
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.
Decreasing version number?
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecs.xml', | ||
'Geometry/GEMGeometryBuilder/data/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v5/GEMSpecs.xml', |
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.
Decreasing version number?
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v7/GEMSpecs.xml', | ||
'Geometry/GEMGeometryBuilder/data/GEMSpecsFilter.xml', | ||
'Geometry/GEMGeometryBuilder/data/v5/GEMSpecs.xml', |
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.
Decreasing version number?
CMSSW coding rules use .h for header files: https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/CodingAndStyleRules.pdf , § 2.1 |
@fwyzard interesting document. I did not know it existed. It could be advertised more for people making PRs :-) |
I get the exception:
@abrinke1 you mentioned that you removed the directory. But there is also a emtf_luts directory in L1Trigger/L1TMuon/data that appears to contain the missing file. How are the two related? |
Hi all, Thanks for all the helpful comments. I haven't responded so far because I was working on getting the EMTF pT assignment with RPCs included, since this is what will be running at P5 when the collisions arrive. You can see the preliminary version in my latest commits (with a fair number of debugging output, and some temporary hacks). I'll be working with @kkotov on Monday to get things all ship-shape with O2O and versioning, and I'll try to address the comments above as well. Best regards, |
Hi all, I've just pushed a working version of the 2017 pT assignment with RPCs, with bit-wise internal debugging on 10k events. The performance looks pretty good. Took me (and Khristian) a bit longer than I expected, so the code cleaning will have to wait for tomorrow. Best, |
THIS PR IS READY TO BE TESTED! @thomreis is there any chance you can try running again, either on a T&P data sample or some MC, and cross-check that the efficiency is OK? @rekovic @bortigno @kkotov @jiafulow please take a look p.s. Just remembered that I didn't address the ".hh" vs. ".h" comment - can deal with this before pulling into CMSSW. |
Superseded by #542 |
Full update of EMTF code, including:
Crucial missing piece:
[1] https://github.com/abrinke1/cmssw/tree/EMTF_April_2017_dev/L1Trigger/L1TMuonEndCap/data
Items to be added:
Major directories affected
DataFormats/L1TMuon/
EventFilter/L1TRawToDigi/
L1Trigger/L1TMuonEndCap/
L1Trigger/L1TMuon
Unusual or unexpected file changes
DQM/L1TMonitor/src/L1TMP7ZeroSupp.cc
EventFilter/L1TRawToDigi/interface/Block.h
Geometry/GEMGeometry/test/ME0GeometryAnalyzer.cc
Geometry/GEMGeometryBuilder/
Geometry/MuonCommonData/
Geometry/MuonNumbering/src/ME0NumberingScheme.cc
Lastly, @rekovic , I still can't see our DQM developer Preston Epps (ICleanPools [2]) anywhere - for example, I can't add him to the Reviewers of this PR. Do you know why this is?
[2] https://github.com/ICleanPools