Skip to content
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 March 2020 emulator update #29262

Closed
wants to merge 18 commits into from
Closed

Conversation

jiafulow
Copy link
Contributor

@jiafulow jiafulow commented Mar 21, 2020

PR description:

This PR adds/updates the interface to all the Phase-2 trigger primitives. This is the first PR from a series of EMTF emulator updates planned for 2020.

The EMTF emulator needs to be upgraded to handle all the Phase-2 trigger primitives, including GEM, ME0, and iRPC. It's also nice to be able to handle DT/TwinMux. Majority of the work has been done during the development for Phase-2 EMTF++, and used for L1T Phase-2 Upgrade TDR studies. These changes are now being ported to the official CMSSW. For Run 3 specifically, the GE1/1 interface is needed to continue the rest of the EMTF-GEM integration.

There are a lot of changes because what's in the existing EMTF emulator related to GEM/ME0/iRPC is not correct/updated. Also, unused codes related to converting track trigger stubs into muon trigger primitives have been removed.

Notes:

  • The definition of Subsector() for EMTFHit converted from RPC hit has changed. It is now consistent with the CSC definition (1 or 2 in ME1, 0 in ME2,3,4). The old definition is no longer stored, but can be recovered via Subsector_RPC().
  • GEM input is now GEMPadDigiClusterCollection.
  • ME0 input is now ME0TriggerDigiCollection.
  • Regarding input: access to simTwinMuxDigis, simDtTriggerPrimitiveDigis, rpcRecHits, me0TriggerConvertedPseudoDigis has been added, but none of them are turned on by default.
  • Regarding output: no change. While the interface has been implemented, the rest of the track finder logic has not been updated to use the new trigger primitives.

Additional notes:

  • No dependence on other PRs or externals.
    • This PR does not interfere with the recent EMTF PRs by Andrew that are related to UL2016 emulation bugs.
  • The plan for EMTF emulator update is discussed here.

PR validation:

From runTheMatrix.py -l limited -i all -j10:

18 17 16 10 4 1 1 1 1 tests passed, 16 0 0 0 0 0 0 0 0 failed
(all the errors are due to DAS ERROR - not sure what's wrong?)

From runTheMatrix.py -w upgrade -l 20434.0,20504.0 -j10:

2 2 2 2 tests passed, 0 0 0 0 failed

Considering this is a big PR, there might be bugs. But we will try to detect and fix them as soon as possible.


Notifications: @abrinke1 @eyigitba @rekovic

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29262/14307

@cmsbuild cmsbuild mentioned this pull request Apr 16, 2020
@rekovic
Copy link
Contributor

rekovic commented Apr 17, 2020

Hi @silviodonato @kpedro88
This PR is Phase-1 oriented as it concerns the current Phase-1 EMTF emulator. It is not the Phase-2 EMTF hardware emulator which will eventually come as a separate package. This is the adjustment to the current Phase-1 emulator to be able to read input TPs from some Phase-2 detectors, one of them being GEMs which are going to be used in Run-3. This is only a preparation step needed to adjust the current emulator be able to consume GEMs for Run-3. It will remain backward compatible to Run-2 data and MC.

@kpedro88
Copy link
Contributor

@rekovic that doesn't seem quite in line with the PR description, which mentions Phase 2 and TDR studies several times

@rekovic
Copy link
Contributor

rekovic commented Apr 17, 2020

@kpedro88
This package L1Trigger/L1TMuonEndCap will remain there and L1TMuonEndCapTrackProduceris to serve for emulation of the Muon Track Finder algorithm running in Phase-1 hardware for Run2. We will use the same hardware in Run3 and this producer will be adjusted for the consumption of GEMs, but will however use the same type of pattern recognition algorithm.

The new hardware used in Phase-2 will support a different algorithm which will be emulated by a different code (not yet ready for integration). I would like to keep the two separate and avoid possibilities of disrupting development of Phase-2 or endangering backward compatibilities.

What is written in this PR description only has to do with the interface, namely ability to read the TPs.
Till the Phase-2 algorithm is emulation is integrated, the Phase-1 producer of standalone muon-tracks will be used for building of Phase2 correlated object (TrackerTrack + Muon).

This was discussed with @jiafulow.

@jiafulow
Copy link
Contributor Author

@kpedro88
I'm used to call these "Phase-2" trigger primitives, although GE1/1 has been installed during this shutdown and GE2/1 will be installed before Phase-2. So, the GEM TPs are more like Phase-1.5. In any case, as Vladimir explained, this PR allows the EMTF to receive the new TPs (GEM, iRPC, ME0), but the rest of the algorithm has not been updated to use them yet. That will happen in the future PRs, and development for the Phase-2 algorithm will be kept separated from L1Trigger/L1TMuonEndCap to avoid confusion.

#include "DataFormats/RPCDigi/interface/RPCDigi.h"
#include "DataFormats/GEMDigi/interface/GEMPadDigi.h"
#include "DataFormats/GEMDigi/interface/ME0PadDigi.h"
//#include "DataFormats/RPCDigi/interface/RPCDigi.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented-out code

RPCDetId CreateRPCDetId() const;
// void ImportGEMDetId (const GEMDetId& _detId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented-out code

int Subsystem() const { return subsystem; }

// See L1Trigger/L1TMuon/interface/MuonTriggerPrimitive.h
bool Is_DT() const { return subsystem == 0; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like a good case for an enum

void set_RoadIdx(unsigned int bits) { _RoadIdx = bits; }
EMTFRoad Road() const { return _Road; }
unsigned int RoadIdx() const { return _RoadIdx; }
//void set_Road(const EMTFRoad& bits) { _Road = bits; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented-out code

@@ -177,15 +177,22 @@ namespace l1t {
RPC_.Word(),
RPC_.Link());

// Rotate by 20 deg to match RPC convention in CMSSW
int _sector_rpc = (_subsector < 5) ? _sector : (_sector % 6) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic numbers should be named constants (everywhere)

int tp_endcap = (tp_region == -1) ? 2 : tp_region;
int tp_station = tp_detId.station();
int tp_ring = 1; // tp_detId.ring() does not exist
//int tp_roll = tp_detId.roll();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented-out code

bool sub5deg = false;
if (includeNeighbor_) {
if ((endcap_ == tp_endcap) && (get_other_neighbor(sector_) == tp_sector)) {
if (tp_csc_ID == 1 && tp_endcap == 1 && tp_pad >= (767 - 192)) { // higher 1/4 of chamber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic numbers should be named constants


int tp_bx = tp_data.bx;
int tp_phi = tp_data.radialAngle;
//int tp_phiB = tp_data.bendingAngle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented-out code

collector.extractPrimitives(GEMTag(), iEvent, tokenGEM_, muon_primitives);
if (useCSC_) {
collector.extractPrimitives(emtf::CSCTag(), &geometry_translator_, iEvent, tokenCSC_, muon_primitives);
//collector.extractPrimitives(emtf::CSCTag(), &geometry_translator_, iEvent, tokenCSC_, tokenCSCComparator_, muon_primitives);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented-out code

int get_trigger_sector(int ring, int station, int chamber) {
int result = 0;
if (station > 1 && ring > 1) {
result = ((static_cast<unsigned>(chamber - 3) & 0x7f) / 6) + 1; // ch 3-8->1, 9-14->2, ... 1,2 -> 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic numbers should be named constants

@kpedro88
Copy link
Contributor

@rekovic @jiafulow some of the code rule/style issues in my review seem preexisting/beyond the scope of this PR, so they can be addressed in a subsequent PR. However, I identified at least one bug and some other potential issues with how the PR will work in production, so please take a look.

@rekovic
Copy link
Contributor

rekovic commented Apr 17, 2020

Thanks @kpedro88 .
Commented-out code can be removed indeed even now.

Thanks for the catch of a wrong size array
const char subsystem_names[][4] = {"DT", "CSC", "RPC", "GEM", "ME0"};

Some magic numbers should indeed be turned into named constants. In some cases I would say there might not be much point in doing so.

@kpedro88
Copy link
Contributor

@rekovic there are several reasons to avoid magic numbers in almost all circumstances. using named constants not only makes the code more readable and understandable for future maintainers by labeling numbers by their meaning, but also it enforces consistency. if a number is changed in one place, should the same number be changed somewhere else? how do i know if that 2 is the same as this other 2? this is always a future problem, not a present one, which is why the policy should always be followed whether or not a problem is "foreseen".

@jiafulow
Copy link
Contributor Author

Regarding const char subsystem_names[][4] = {"DT", "CSC", "RPC", "GEM", "ME0"};
In this case, the compiler expands [][4] to [5][4], which is the necessary size for 5 elements that have maximum 3 characters plus 1 `\0' character. It is absolutely correct.

I have no idea that commented out codes are against the CMS rules. Sometimes, codes are commented out for now, but might need to be uncommented in the future. Sometimes codes are commented out to leave as a trace, so that if bugs are found in the future, it's easier to debug by checking what the previous version was. Deleting them just makes life harder for the few developers who have to work with the codes. The codes are almost always work in progress, rather than a finished product. It sounds crazy to me that this is instituted as a rule.

Regarding the magic numbers, most of the cryptic ones are already explained by the comments next to them. The trivial ones are clear from the context. There's no reason to assume any future human maintainers wouldn't be able to figure that out.

@abrinke1
Copy link
Contributor

Hi @kpedro88 ,

I'm all for clarity, but surely when the "magic" number is just the ID number, you don't increase the clarity by creating a new named constant? For example, in the code above "tp_csc_ID == 1" clearly indicates that the CSC ID must equal 1. This would not be made any clearer by defining some new constant:
int CSC_ID_1 = 1;
and then changing the condition to be:
tp_csc_ID == CSC_ID_1;
In fact, it would make the code more confusing. There's no "translation" of CSC ID = 1; the CSC chamber with ID = 1 is just the CSC chamber with ID = 1. I suppose you could add a geometrical description ("the chamber in the bottom left of each sector"), but that would be incredibly tedious.

Cheers,
@abrinke1

@kpedro88
Copy link
Contributor

@jiafulow

I have no idea that commented out codes are against the CMS rules. Sometimes, codes are commented out for now, but might need to be uncommented in the future. Sometimes codes are commented out to leave as a trace, so that if bugs are found in the future, it's easier to debug by checking what the previous version was. Deleting them just makes life harder for the few developers who have to work with the codes. The codes are almost always work in progress, rather than a finished product. It sounds crazy to me that this is instituted as a rule.

If there is a particular reason that a commented-out line of code should be kept, this should be indicated in a separate comment above that line of code. Typically, this is a temporary situation, and therefore the conditions for uncommenting that line of code should be clearly stated.

Otherwise, if a line of code is no longer needed, it is still preserved in the git history (and previous releases) for anyone who needs to look back at it.

CMSSW has 100s of contributors, with people frequently entering and leaving the collaboration. Leaving a file littered with commented-out lines of code makes it significantly harder to tell what the code is supposed to do. Once code is in the official repository, maintaining it is everyone's responsibility, not just the original developer's. Therefore, a higher standard must be met.

@jiafulow and @abrinke1:

Regarding the magic numbers, most of the cryptic ones are already explained by the comments next to them. The trivial ones are clear from the context. There's no reason to assume any future human maintainers wouldn't be able to figure that out.

Making human maintainers "figure things out" that computers can do automatically is bad practice.

In fact, it would make the code more confusing. There's no "translation" of CSC ID = 1; the CSC chamber with ID = 1 is just the CSC chamber with ID = 1. I suppose you could add a geometrical description ("the chamber in the bottom left of each sector"), but that would be incredibly tedious.

What if, for some reason, a future muon subdetector geometry swaps what is currently considered chamber 1 with chamber 2? Do you want to go through thousands of lines of code and figure out which "1"s need to be swapped with which "2"s? This is a somewhat contrived example, but there are plenty of non-contrived examples just in this PR.

@abrinke1
Copy link
Contributor

"What if, for some reason, a future muon subdetector geometry swaps what is currently considered chamber 1 with chamber 2? Do you want to go through thousands of lines of code and figure out which "1"s need to be swapped with which "2"s?"

My answer would be: "absolutely, yes." Because there is no central CMSSW object linking physical chambers to their numerical IDs, and if we decided to create one now, we'd have to go through all the existing L1T and CSC code and update it to use this chamber-map object. If we don't create such an object, then any change to the mapping means we have to go through all the code and find all of the new constants we created and change them by hand anyway. It saves 0 time, and adds 0 clarity compared to just using the ID number.

@silviodonato
Copy link
Contributor

In https://cms-sw.github.io/cms_coding_rules.html you can find the "CMSSW rules".
About magic numbers, it is a generic C++ code style rules. In https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enum1-prefer-enumerations-over-macros
you can find a trivial example

constexpr int first_month = 1;
constexpr int last_month = 12;
for (int m = first_month; m <= last_month; ++m) 
    cout << month[m] << '\n';

is more understandable than

for (int m = 1; m <= 12; ++m)
    cout << month[m] << '\n';

The code has exactly the same performance (last_month is replaced with 12 at compile time).
Therefore, we suggest to use constexpr constants instead of numbers and comments.
Clearly, this is not a huge problem, but we should try to improve the code quality of CMSSW.

About

const bool is_irpc = (tp_station == 3 || tp_station == 4) && (tp_ring == 1);

I think it is rather clear, but the following code is more understandable for people who are not L1T experts

constexpr auto RE3_1 =  std::pair<int, int>(3, 1) //station, ring
constexpr auto RE4_1 =  std::pair<int, int>(4, 1) //station, ring
[...]
const auto module = std::pair<int, int>(tp_station, tp_ring)
const bool is_irpc = ( module == RE3_1 ) ||( module == RE4_1 )

Anyhow, you can keep the 1-line version, if you strongly prefer it.

About

result = ((static_cast<unsigned>(chamber - 3) & 0x7f) / 6) + 1;  // ch 3-8->1, 9-14->2, ... 1,2 -> 6

you definitely need to use constexpr variable for 0x7fand all other numbers.

Regarding the commented code, I confirm the policy of deleting commented lines.
We can retrieve any version of the old code from GitHub.
Commented line should be used only for temporary changes.
You are right, this rule does not appear in "CMS Naming, Coding, And Style Rules". We are going to update them.

@jiafulow
Copy link
Contributor Author

jiafulow commented Apr 20, 2020

Let me be clear. No one in our group who is responsible for developing/maintaining this particular package would have trouble understanding

for (int m = 1; m <= 12; ++m)
    cout << month[m] << '\n';

or

const bool is_irpc = (tp_station == 3 || tp_station == 4) && (tp_ring == 1);

The hypothetical scenario where a maintainer would struggle to understand them doesn't exist. Not now, not in the future.

I'm sorry I simply don't agree that the following code is better:

constexpr auto RE3_1 =  std::pair<int, int>(3, 1) //station, ring
constexpr auto RE4_1 =  std::pair<int, int>(4, 1) //station, ring
[...]
const auto module = std::pair<int, int>(tp_station, tp_ring)
const bool is_irpc = ( module == RE3_1 ) ||( module == RE4_1 )

It's introducing throwaway variables, and it's harder to read than a one-liner. Everyone has different opinions, I've stated my opinions, and I'm not going to argue further on what is 'better' or higher code quality. In the end, our group is the one that takes responsibilities of the output of this particular package. As the developer, I have my own opinions and I am not going to be force-fed bullshit opinions. My job is to submit a PR that does what the L1 group asked of us. If the PR is not doing what is described, I'm happy to fix it. But I'm not entertaining pointless code review comments.

Regarding result = ((static_cast<unsigned>(chamber - 3) & 0x7f) / 6) + 1; // ch 3-8->1, 9-14->2, ... 1,2 -> 6
it was taken from https://github.com/cms-sw/cmssw/blob/master/DataFormats/MuonDetId/src/CSCDetId.cc#L11-L20
If you are really unhappy with the magic numbers, please fix that piece of code and then I'll copy from the updated code.

Calling the commented-out codes 'litter' is also a stretch. Most of them are single-line, and only a few of them are 3-4 lines. They don't make it 'significantly harder' to read at all.

@kpedro88
Copy link
Contributor

But I'm not entertaining pointless code review comments.

Please note that no developer or group is entitled to have their code included in the official repository. Your group may "take responsibility" for the code in an informal way, but once it is included in the official repository, it is everyone's responsibility - if it breaks, it must be fixed.

I deliberately included this statement in my PR review to indicate the relative importance of comments: "some of the code rule/style issues in my review seem preexisting/beyond the scope of this PR, so they can be addressed in a subsequent PR." But I am also happy to reject the PR outright if the developers feel they don't need to follow the rules at all.

@jiafulow
Copy link
Contributor Author

jiafulow commented Apr 20, 2020

it is everyone's responsibility - if it breaks, it must be fixed.

Except that every time the EMTF emulator broke, it was up to us to fix it.

If you feel you have the power over all the codes that we contributed, and do not want to address any concerns that your rules are bad and sometimes even pointless, then go ahead and do what you want to do. I've already provided the justifications for the select lines of codes you guys wanted to discuss. There's no reason for me to fix anything that is not broken.

@kpedro88
Copy link
Contributor

-1
developers are belligerent and do not understand how software management works in a large collaboration
this behavior should not be rewarded

@rekovic
Copy link
Contributor

rekovic commented Apr 21, 2020

Coming back to this thread.

@kpedro88

@rekovic there are several reasons to avoid magic numbers in almost all circumstances. using named constants not only makes the code more readable and understandable for future maintainers by labeling numbers by their meaning, but also it enforces consistency. if a number is changed in one place, should the same number be changed somewhere else? how do i know if that 2 is the same as this other 2? this is always a future problem, not a present one, which is why the policy should always be followed whether or not a problem is "foreseen".

I agree. For the sake of consistency using constants is preferred. In some cases this could severely jeopardizes the readability, but in the long run it pays off.

@rekovic
Copy link
Contributor

rekovic commented Apr 21, 2020

Hi @jiafulow
The requests of @kpedro88 should be addressed.

  • if indeed needed to keep commented out lines perhaps add description. This all can be cleaned up in the subsequent PR's part of this series when the debugging is finished.
  • where possible please use named constants

@silviodonato
Copy link
Contributor

This PR has conflicts, it needs to be rebased.
CMSSW_11_1_0_pre7 will be close tomorrow.

@silviodonato
Copy link
Contributor

moved to #29767

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants