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

Latest GT development #1183

Conversation

HaarigerHarald
Copy link

Hi,

I was told to open a PR to merge our latest GT development into the phase2-l1t-integration branch.

This includes:

  • The step-1 menu (from @jheikkil)
  • ntupelizer (from @jheikkil)
  • Vertex dZ cuts
  • m/dR cuts
  • Multi-body cuts (3 and 4 body invariant + transvers mass)
  • Output object pattern writer
  • FinalOR emulator
  • GMT's charge definition
  • TMUX 2 output pattern structure

@triggerDoctor
Copy link

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

Attempts to compile this PR failed.

The following is the stderr of the compilation attempt:

/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc: In member function 'void L1Analysis::L1AnalysisPhaseIIStep1::SetTkEG(edm::Handle<std::vector<l1t::TkElectron> >, unsigned int)':
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:178:38: error: 'const class l1t::TkElectron' has no member named 'trackCurvature'
  178 |       int chargeFromCurvature = (it->trackCurvature() > 0) ? 1 : -1;  // ThisIsACheck
      |                                      ^~~~~~~~~~~~~~
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:185:48: error: 'const class l1t::TkElectron' has no member named 'EGRef'
  185 |       l1extra_.tkElectronEGRefPt.push_back(it->EGRef()->et());  //Rename  this?
      |                                                ^~~~~
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:186:49: error: 'const class l1t::TkElectron' has no member named 'EGRef'
  186 |       l1extra_.tkElectronEGRefEta.push_back(it->EGRef()->eta());
      |                                                 ^~~~~
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:187:49: error: 'const class l1t::TkElectron' has no member named 'EGRef'
  187 |       l1extra_.tkElectronEGRefPhi.push_back(it->EGRef()->phi());
      |                                                 ^~~~~
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc: In member function 'void L1Analysis::L1AnalysisPhaseIIStep1::SetTkEM(edm::Handle<std::vector<l1t::TkEm> >, unsigned int)':
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:252:46: error: 'const class l1t::TkEm' has no member named 'EGRef'
  252 |       l1extra_.tkPhotonEGRefPt.push_back(it->EGRef()->et());  //REname this?
      |                                              ^~~~~
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:253:47: error: 'const class l1t::TkEm' has no member named 'EGRef'
  253 |       l1extra_.tkPhotonEGRefEta.push_back(it->EGRef()->eta());
      |                                               ^~~~~
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:254:47: error: 'const class l1t::TkEm' has no member named 'EGRef'
  254 |       l1extra_.tkPhotonEGRefPhi.push_back(it->EGRef()->phi());
      |                                               ^~~~~
gmake: *** [config/SCRAM/GMake/Makefile.rules:1793: tmp/slc7_amd64_gcc11/src/L1Trigger/L1TNtuples/src/L1TriggerL1TNtuples/L1AnalysisPhaseIIStep1.cc.o] Error 1
gmake: *** Waiting for unfinished jobs....

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

@triggerDoctor
Copy link

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

This PR failed the code checks.

I found the following lines where an "error" was mentioned, they may help in debugging

/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:178:38: error: no member named 'trackCurvature' in 'l1t::TkElectron' [clang-diagnostic-error]
...
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:185:48: error: no member named 'EGRef' in 'l1t::TkElectron' [clang-diagnostic-error]
...
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:186:49: error: no member named 'EGRef' in 'l1t::TkElectron' [clang-diagnostic-error]
...
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:187:49: error: no member named 'EGRef' in 'l1t::TkElectron' [clang-diagnostic-error]
...
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:252:46: error: no member named 'EGRef' in 'l1t::TkEm' [clang-diagnostic-error]
...
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:253:47: error: no member named 'EGRef' in 'l1t::TkEm' [clang-diagnostic-error]
...
/tmp/palencia/pr_1183/new/src/L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:254:47: error: no member named 'EGRef' in 'l1t::TkEm' [clang-diagnostic-error]
...

Please check and see if these lines help debugging.

Info Value
return code 2
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
Copy link

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

I found 4 files that did not meet formatting requirements:

  • L1Trigger/L1TNtuples/interface/L1AnalysisPhaseIIStep1DataFormat.h
  • L1Trigger/L1TNtuples/plugins/L1PhaseIITreeStep1Producer.cc
  • L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.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

@HaarigerHarald
Copy link
Author

@jheikkil can I remove the ntupelizer or do you want to update it?

@jheikkil
Copy link

@jheikkil can I remove the ntupelizer or do you want to update it?

Hi @HaarigerHarald , please go ahead.

@triggerDoctor
Copy link

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
Copy link

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
Copy link

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/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

@aloeliger aloeliger added Phase-2 Pertains to phase-2 development Emulator Development Emulator development PR labels Nov 29, 2023
Comment on lines +315 to +316
class L1GTAlgoBlockProducer
: public edm::one::EDProducer<edm::RunCache<std::unordered_map<std::string, unsigned int>>> {

Choose a reason for hiding this comment

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

Does this need to be one? Maybe I didn't spot the deal breaker and haven't done the full reasoning about thread safety admittedly, but could this be made into an edm::global::EDProducer<edm::RunCache<std::unordered_map<std::string, unsigned int>>>? This would be a little more efficient on stream/thread resources is my understanding from here

Copy link
Author

Choose a reason for hiding this comment

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

It sort of needs to be a one otherwise the prescaling is non-deterministic and would depend on which thread executes first. For execution this is probably fine and I think the HLT even uses something similar. However, it would be horrible if we want to compare emulator with firmware.

Choose a reason for hiding this comment

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

Alright, sounds good.

res &= minDRSquared_ ? dRSquared > minDRSquared_ : true;
res &= maxDRSquared_ ? dRSquared < maxDRSquared_ : true;
}

res &= os_ ? obj1.hwCharge() != obj2.hwCharge() : true;
res &= ss_ ? obj1.hwCharge() == obj2.hwCharge() : true;

int32_t lutCoshDEta = 0;

Choose a reason for hiding this comment

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

Our script doesn't catch this yet (apparently), but you may get uninitialized variable complaints from build bot here in the long term. If it isn't breaking, it may be good to try and initialize this to something default to start. Same with the phi version.

// Optimization: (cos(x + pi) = -cos(x), x in [0, pi))
int32_t lutCosDPhi = dPhi >= HW_PI ? -cosPhiLUT_[dPhi] : cosPhiLUT_[dPhi];

int64_t invMassSqrDiv2;

Choose a reason for hiding this comment

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

If possible you may want to initialize this.

Comment on lines +174 to +184
unsigned int atIndex(int objeta) const {
// Function that checks at which index the eta of the object is
// If object abs(eta) < regionsAbsEtaLowerBounds_[0] the function returns the last index,
// Might be undesired behaviour
for (unsigned int i = 0; i < regionsAbsEtaLowerBounds_.size() - 1; i++) {
if (std::abs(objeta) >= regionsAbsEtaLowerBounds_[i] && std::abs(objeta) < regionsAbsEtaLowerBounds_[i + 1]) {
return i;
}
}
return regionsAbsEtaLowerBounds_.size() - 1;
}

Choose a reason for hiding this comment

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

I would guess the performance details of this don't matter, but would it be better to binary search this instead of linearly?

@@ -1,6 +1,9 @@
import FWCore.ParameterSet.Config as cms
import re


TYPE_STANDARD_PHYSICS = cms.vint32(1) # TODO

Choose a reason for hiding this comment

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

Does this TODO need to be implemented?

Copy link
Author

Choose a reason for hiding this comment

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

Trigger types are still open for discussion (see TDR). We'll decide on this at a later date what the actual trigger types should be.

tag = cms.InputTag("l1tGTProducer", "GMTTkMuons"),
minEta = cms.double(-1.5),
maxEta = cms.double(1.5),
qual = cms.vuint32(0b00000001, 0b00000010, 0b00000011, 0b00000100, 0b00000101, 0b00000110, 0b00000111, 0b00001000, 0b00001001, 0b00001010, 0b00001011, 0b00001100, 0b00001101, 0b00001110, 0b00001111, 0b00010000, 0b00010001, 0b00010010, 0b00010011, 0b00010100, 0b00010101, 0b00010110, 0b00010111, 0b00011000, 0b00011001, 0b00011010, 0b00011011, 0b00011100, 0b00011101, 0b00011110, 0b00011111, 0b00100000, 0b00100001, 0b00100010, 0b00100011, 0b00100100, 0b00100101, 0b00100110, 0b00100111, 0b00101000, 0b00101001, 0b00101010, 0b00101011, 0b00101100, 0b00101101, 0b00101110, 0b00101111, 0b00110000, 0b00110001, 0b00110010, 0b00110011, 0b00110100, 0b00110101, 0b00110110, 0b00110111, 0b00111000, 0b00111001, 0b00111010, 0b00111011, 0b00111100, 0b00111101, 0b00111110, 0b00111111, 0b01000000, 0b01000001, 0b01000010, 0b01000011, 0b01000100, 0b01000101, 0b01000110, 0b01000111, 0b01001000, 0b01001001, 0b01001010, 0b01001011, 0b01001100, 0b01001101, 0b01001110, 0b01001111, 0b01010000, 0b01010001, 0b01010010, 0b01010011, 0b01010100, 0b01010101, 0b01010110, 0b01010111, 0b01011000, 0b01011001, 0b01011010, 0b01011011, 0b01011100, 0b01011101, 0b01011110, 0b01011111, 0b01100000, 0b01100001, 0b01100010, 0b01100011, 0b01100100, 0b01100101, 0b01100110, 0b01100111, 0b01101000, 0b01101001, 0b01101010, 0b01101011, 0b01101100, 0b01101101, 0b01101110, 0b01101111, 0b01110000, 0b01110001, 0b01110010, 0b01110011, 0b01110100, 0b01110101, 0b01110110, 0b01110111, 0b01111000, 0b01111001, 0b01111010, 0b01111011, 0b01111100, 0b01111101, 0b01111110, 0b01111111, 0b10000000, 0b10000001, 0b10000010, 0b10000011, 0b10000100, 0b10000101, 0b10000110, 0b10000111, 0b10001000, 0b10001001, 0b10001010, 0b10001011, 0b10001100, 0b10001101, 0b10001110, 0b10001111, 0b10010000, 0b10010001, 0b10010010, 0b10010011, 0b10010100, 0b10010101, 0b10010110, 0b10010111, 0b10011000, 0b10011001, 0b10011010, 0b10011011, 0b10011100, 0b10011101, 0b10011110, 0b10011111, 0b10100000, 0b10100001, 0b10100010, 0b10100011, 0b10100100, 0b10100101, 0b10100110, 0b10100111, 0b10101000, 0b10101001, 0b10101010, 0b10101011, 0b10101100, 0b10101101, 0b10101110, 0b10101111, 0b10110000, 0b10110001, 0b10110010, 0b10110011, 0b10110100, 0b10110101, 0b10110110, 0b10110111, 0b10111000, 0b10111001, 0b10111010, 0b10111011, 0b10111100, 0b10111101, 0b10111110, 0b10111111, 0b11000000, 0b11000001, 0b11000010, 0b11000011, 0b11000100, 0b11000101, 0b11000110, 0b11000111, 0b11001000, 0b11001001, 0b11001010, 0b11001011, 0b11001100, 0b11001101, 0b11001110, 0b11001111, 0b11010000, 0b11010001, 0b11010010, 0b11010011, 0b11010100, 0b11010101, 0b11010110, 0b11010111, 0b11011000, 0b11011001, 0b11011010, 0b11011011, 0b11011100, 0b11011101, 0b11011110, 0b11011111, 0b11100000, 0b11100001, 0b11100010, 0b11100011, 0b11100100, 0b11100101, 0b11100110, 0b11100111, 0b11101000, 0b11101001, 0b11101010, 0b11101011, 0b11101100, 0b11101101, 0b11101110, 0b11101111, 0b11110000, 0b11110001, 0b11110010, 0b11110011, 0b11110100, 0b11110101, 0b11110110, 0b11110111, 0b11111000, 0b11111001, 0b11111010, 0b11111011, 0b11111100, 0b11111101, 0b11111110, 0b11111111),

Choose a reason for hiding this comment

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

Instead of repeating this for several seeds, could some other variable be declared for this and an explanation added for what these bit patterns represent?

Choose a reason for hiding this comment

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

Hi @aloeliger ! As you might be aware, there is a discussion ongoing between the GT and the upstream systems related to the usage and format of quality flags for upstream objects. The P2GT team has implemented this as a temporary solution, simply to be able to run the menu. In case of track-matched muons, these flags represent implementing "quality>0" as we were instructed to do. Concerning your proposal, I will let @HaarigerHarald and Elias (i cannot find his nickname) comment.

@aloeliger
Copy link

This will be held by GT team request until notified.

@HaarigerHarald HaarigerHarald force-pushed the phase2-l1t-integration-13_3_0_pre3 branch from 929bdce to 42ed293 Compare January 23, 2024 14:12
A higher resolution doesn't make sense with our current cosh/cos resolution
@aloeliger 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:38
@aloeliger aloeliger changed the base branch from phase2-l1t-integration-14_0_0_pre3 to phase2-l1t-integration-13_3_0_pre3 February 13, 2024 10:49
@aloeliger aloeliger added the Needs Rebase PR should be rebased to newer branch label Feb 13, 2024
@aloeliger
Copy link

@HaarigerHarald This branch needs to be rebased to CMSSW_14_0_0_pre3. The series of steps I would attempt to do that are:

  1. First, make a backup of this branch and it's commits pushed to github under a different name
  2. checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
  3. Get a copy of this branch locally via git cms-checkout-topic -u p2l1-gtEmulator:phase2-l1t-integration-13_3_0_pre3
  4. Perform an interactive rebase via git rebase from-CMSSW_14_0_0_pre3 --interactive
  5. 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 commits from track trigger commits that get caught up in this somehow?
    5. b. There seems to be conflicts between this PR and the integration branch already. Please resolve them.
  6. 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

@HaarigerHarald
Copy link
Author

I'll close this and open a new one from p2l1-gtEmulator:phase2-l1t-integration-14_0_0_pre3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge Emulator Development Emulator development PR Needs Rebase PR should be rebased to newer branch Phase-2 Pertains to phase-2 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants