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

Updates of the uGT emulator and L1Ntuple workflows for ZDC signals #42635

Closed

Conversation

elfontan
Copy link
Contributor

@elfontan elfontan commented Aug 22, 2023

PR description:

This draft PR includes an update of the uGT emulator for new ZDC triggers included in the Heavy Ion menu for the 2023 data-taking. The implementation relies on new developments included in the utm libraries v0_11_0.

The main changes are the following:

  • The TriggerMenuParser is adapted to parse the new ZDCPlus and ZDCMinus conditionTypes from the xml menu. A dedicated parsing function parseZdcEnergySum is included to relate these objects to the new ZdcEnergySumTemplate.
  • The GlobalBoard is updated to receive a new category of objects similar to the EtSum from the CaloLayer2 coming from the new ZDC Producer included in L1T ZDC and uGT Emulator to CMSSW_13_3_X #42634.
  • A dedicated ZdcEnergySumTemplate and a ZdcEnergySumCondition class are developed for checking the EtSum threshold on the ZDC objects.
  • The ste of input tags to the L1TGlobalProducer is updated in simGtStage2Digis_cfi.py.

The following L1Ntuples workflows are also updated to include the new input tags for ZDC objects:

  • L1NtupleEMU_cff.py
  • L1NtupleRAW_cff.py
  • l1UpgradeTree_cfi.py

Still unclear how we need to update the customiseReEmul.py sequence to include the L1ZDCProducer. Current tests have been done manually adding to the configuration file the following lines:

process.zdcEtSumProducer = cms.EDProducer('L1TZDCProducer',
  zdcToken = cms.InputTag("hcalDigis", "ZDC"),
  doHardCodeLUT = cms.bool(True)
)

process.zdcEtSum = cms.Path(process.zdcEtSumProducer)
process.schedule.append(process.zdcEtSum)

PR validation:

All these changes have been added on top of the updates in #42634 following the recipe in the README. A configuration file for a 2018 HI run has been obtained following the recipe provided by the HI team.
The produced L1Ntuple stores the new ZDCEnergySums.
Countings for the ZDC algorithms are present in the uGTEmuTree. However, they are currently based on the comparison with a dummy energy value included in the ZdcEnergySumCondition class. A missing piece to get the energy sum computed in the ZDCProducer into the uGT is still under investigation.

The PR has been prepared starting from CMSSW_13_3_X_2023-08-22-1100:

cmsrel CMSSW_13_3_X_2023-08-22-1100
cd CMSSW_13_3_X_2023-08-22-1100/src
cmsenv
git cms-init
scram b && scram b code-checks && scram b code-format && scram b

Note that the updates to the DataFormats from #42634 are included here as well to avoid the compilation crashes. These changes are overlapping at the moment. We need to understand how to disentangle the two PRs, if possible, or just create a single one.

Additional notes:

A backport to CMSSW_13_2_X will be needed as that is the release designated for the 2023 HI data-taking.

Tagging relevant people for:

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36692

ERROR: Build errors found during clang-tidy run.

      type = l1t::EtSum::EtSumType::kZDCP;
             ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:201:10: error: use of undeclared identifier 'gtZDCM' [clang-diagnostic-error]
    case gtZDCM:
         ^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:202:37: error: no member named 'kZDCM' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
      type = l1t::EtSum::EtSumType::kZDCM;
             ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:213:38: error: no member named 'kZDCP' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  if (type == l1t::EtSum::EtSumType::kZDCP || type == l1t::EtSum::EtSumType::kZDCM) {
              ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:213:78: error: no member named 'kZDCM' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  if (type == l1t::EtSum::EtSumType::kZDCP || type == l1t::EtSum::EtSumType::kZDCM) {
                                                      ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:230:38: error: no member named 'kZDCP' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  if (type == l1t::EtSum::EtSumType::kZDCP) {
              ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:233:45: error: no member named 'kZDCM' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  } else if (type == l1t::EtSum::EtSumType::kZDCM) {
                     ~~~~~~~~~~~~~~~~~~~~~~~^
Suppressed 820 warnings (820 in non-user code).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36701

ERROR: Build errors found during clang-tidy run.

      type = l1t::EtSum::EtSumType::kZDCP;
             ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:201:10: error: use of undeclared identifier 'gtZDCM' [clang-diagnostic-error]
    case gtZDCM:
         ^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:202:37: error: no member named 'kZDCM' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
      type = l1t::EtSum::EtSumType::kZDCM;
             ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:213:38: error: no member named 'kZDCP' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  if (type == l1t::EtSum::EtSumType::kZDCP || type == l1t::EtSum::EtSumType::kZDCM) {
              ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:213:78: error: no member named 'kZDCM' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  if (type == l1t::EtSum::EtSumType::kZDCP || type == l1t::EtSum::EtSumType::kZDCM) {
                                                      ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:230:38: error: no member named 'kZDCP' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  if (type == l1t::EtSum::EtSumType::kZDCP) {
              ~~~~~~~~~~~~~~~~~~~~~~~^
L1Trigger/L1TGlobal/src/ZdcEnergySumCondition.cc:233:45: error: no member named 'kZDCM' in 'l1t::EtSum::EtSumType' [clang-diagnostic-error]
  } else if (type == l1t::EtSum::EtSumType::kZDCM) {
                     ~~~~~~~~~~~~~~~~~~~~~~~^
Suppressed 820 warnings (820 in non-user code).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36702

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @elfontan (Elisa Fontanesi) for master.

It involves the following packages:

  • DataFormats/L1TGlobal (l1)
  • DataFormats/L1Trigger (l1)
  • L1Trigger/L1TGlobal (l1)
  • L1Trigger/L1TNtuples (l1)

@epalencia, @cmsbuild, @aloeliger can you please review it and eventually sign? Thanks.
@kreczko, @rovere, @eyigitba, @Martin-Grunewald, @missirol, @thomreis, @dinyar this is something you requested to watch as well.
@perrotta, @dpiparo, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@@ -103,6 +105,7 @@ L1TGlobalProducer::L1TGlobalProducer(const edm::ParameterSet& parSet)
m_tauInputTag(parSet.getParameter<edm::InputTag>("TauInputTag")),
m_jetInputTag(parSet.getParameter<edm::InputTag>("JetInputTag")),
m_sumInputTag(parSet.getParameter<edm::InputTag>("EtSumInputTag")),
m_zdcEtSumInputTag(parSet.getParameter<edm::InputTag>("ZdcEtSumInputTag")),
Copy link
Contributor

Choose a reason for hiding this comment

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

What should be the value of this new InputTag in the instance of the L1T emulator used inside the HLT menu [1] ?

As far as I understand, in principle it should be a product coming from the L1T unpacker, e.g. something like "hltGtStage2Digis:ZdcEtSum", but I'm not seeing where this is produced. Is there a L1T unpacker for ZDC ? (sorry if the question is naive)

[1]

process.hltGtStage2ObjectMap = cms.EDProducer( "L1TGlobalProducer",
    MuonInputTag = cms.InputTag( 'hltGtStage2Digis','Muon' ),
    MuonShowerInputTag = cms.InputTag( 'hltGtStage2Digis','MuonShower' ),
    EGammaInputTag = cms.InputTag( 'hltGtStage2Digis','EGamma' ),
    TauInputTag = cms.InputTag( 'hltGtStage2Digis','Tau' ),
    JetInputTag = cms.InputTag( 'hltGtStage2Digis','Jet' ),
    EtSumInputTag = cms.InputTag( 'hltGtStage2Digis','EtSum' ),
    ExtInputTag = cms.InputTag( "hltGtStage2Digis" ),
    AlgoBlkInputTag = cms.InputTag( "hltGtStage2Digis" ),
    GetPrescaleColumnFromData = cms.bool( False ),
    AlgorithmTriggersUnprescaled = cms.bool( True ),
    RequireMenuToMatchAlgoBlkInput = cms.bool( True ),
    AlgorithmTriggersUnmasked = cms.bool( True ),
    useMuonShowers = cms.bool( True ),
    resetPSCountersEachLumiSec = cms.bool( True ),
    semiRandomInitialPSCounters = cms.bool( False ),
    ProduceL1GtDaqRecord = cms.bool( True ),
    ProduceL1GtObjectMapRecord = cms.bool( True ),
    EmulateBxInEvent = cms.int32( 1 ),
    L1DataBxInEvent = cms.int32( 5 ),
    AlternativeNrBxBoardDaq = cms.uint32( 0 ),
    BstLengthBytes = cms.int32( -1 ),
    PrescaleSet = cms.uint32( 1 ),
    Verbosity = cms.untracked.int32( 0 ),
    PrintL1Menu = cms.untracked.bool( False ),
    TriggerMenuLuminosity = cms.string( "startup" )
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @missirol, for the moment we are not having (and we are not going to have in a very short term) an unpacker for ZDC. The plan was to implement something at a later stage (as discussed with @aloeliger @dinyar and HI people). Most likely the inputTag coming from L1 and used in the HLT menu will be the same one that we are going to use in the uGT, that after some discussion it looks that it will be simCaloStage2Digis. The idea is to save the EtSum value from the ZDC producer in the same output collection that contains all EtSum values from CaloLayer2. I ask @bundocka or any other L1 expert to correct me if I am wrong.

Copy link
Contributor

@Martin-Grunewald Martin-Grunewald Aug 24, 2023

Choose a reason for hiding this comment

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

simCaloStage2Digis looks like simulation - what about real data?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that sim refers to the fact that we are simulating the trigger decision, but the inputs are real data - I've put a snippet from an edmConfigDump below of a python file I run over 2018 PbPb data to test the code. Please correct me if I've misunderstood, however

`
process.simCaloStage2Digis = cms.EDProducer("L1TStage2Layer2Producer",
firmware = cms.int32(1),
towerToken = cms.InputTag("simCaloStage2Layer1Digis"),
useStaticConfig = cms.bool(False)
)

process.simCaloStage2Layer1Digis = cms.EDProducer("L1TCaloLayer1",
ecalToken = cms.InputTag("ecalDigis","EcalTriggerPrimitives"),
firmwareVersion = cms.int32(3),
hcalToken = cms.InputTag("hcalDigis"),
unpackEcalMask = cms.bool(False),
unpackHcalMask = cms.bool(False),
useCalib = cms.bool(True),
useECALLUT = cms.bool(True),
useHCALFBLUT = cms.bool(False),
useHCALLUT = cms.bool(True),
useHFLUT = cms.bool(True),
useLSB = cms.bool(True),
verbose = cms.untracked.bool(False)
)
`

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is to save the EtSum value from the ZDC producer in the same output collection that contains all EtSum values from CaloLayer2

What happens if one run the L1T unpackers and uGT emulator on data that does not contain the ZDC ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Andrea,
my understanding of the L1 Trigger emulation is that unpackers for objects are running only if there is something to unpack (= ZDC FED present in the run). I let L1T experts comment more though.

On my end, I quickly tried to run the current emulator on a data file from 2022A (that doesn't contain ZDC) but I have an error at earlier emulation stages. Not sure if that's relevant.

----- Begin Fatal Exception 24-Aug-2023 22:00:55 CEST-----------------------
An exception of category 'Conditions mismatch' occurred while
   [0] Processing  Event run: 362219 lumi: 1 event: 14289 stream: 0
   [1] Running path 'L1TReEmulPath'
   [2] Prefetching for module HcalTrigPrimDigiProducer/'simHcalTriggerPrimitiveDigis'
   [3] Calling method for EventSetup module HcalTPGCoderULUT/''
Exception Message:
Requested conditions of type HcalChannelQuality for cell (0x43280401) (HB 1,1,2) got conditions for cell (0x0) 
----- End Fatal Exception -------------------------------------------------

Any suggestion about how we can test this effect (and prevent issues) more in detail?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cms-sw/l1-l2 can you suggest how to try running this version of the uGT emulator on older data that does not contain the ZDC ?

iEvent.getByToken(zdcEtSumInputToken, etSumData);

if (!etSumData.isValid()) {
if (m_verbosity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, the L1T emulator of the HLT menu would get here, then not issue LogWarnings because the verbosity level is set to 0 at HLT, so an invalid InputTag for this ZDCEtSum basically goes undetected (?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that what it is needed by HLT is the uGT emulator output, so it means that the L1TGlobalProducer when receiving the Calo Object Data (receiveCaloObjectData) will get there, exactly. What happens in case an invalid InputTag is provided, I don't know... As far as I can tell, at the moment the code is not breaking (and I am unsure if we are providing a valid but somehow wrong input tag, or an invalid input tag).

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if full verbosity were enabled, the logs from the HLT are basically ignored, so nobody would see the message except maybe during testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

But anyway this is the same approach used for all other collections...

@missirol
Copy link
Contributor

please test

I'm far from grasping the content of this PR, but I would like to see what happens with the PR tests.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4995a3/34457/summary.html
COMMIT: 4fd8681
CMSSW: CMSSW_13_3_X_2023-08-23-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42635/34457/install.sh to create a dev area with all the needed externals and cmssw changes.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@elfontan
Copy link
Contributor Author

Thanks @missirol for the comments and for triggering the additional tests.
Regarding the failures: we are planning to move the entire ZDC code in a single PR merging #42634 and #42635 in order to avoid issues with failed tests due to the dependencies on the other PR.

}

// destructor
ZdcEnergySumTemplate::~ZdcEnergySumTemplate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove and declare = default (or remove entirely)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36754

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36755

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36756

@cmsbuild
Copy link
Contributor

Pull request #42635 was updated. @epalencia, @cmsbuild, @aloeliger can you please check and sign again.

@elfontan elfontan force-pushed the l1tZDCEmulator_uGT_CMSSW13_3_X branch from 8a5296d to ef810e2 Compare August 29, 2023 02:40
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42635/36761

@cmsbuild
Copy link
Contributor

Pull request #42635 was updated. @epalencia, @cmsbuild, @aloeliger can you please check and sign again.

cfmcginn added a commit to cfmcginn/cmssw that referenced this pull request Aug 29, 2023
…after L1TCalorimeter

Updated naming scheme to remove Stage2 in the ZDC producer - also added contact info and dates in event of bugs/history issues

Stripped out everything except for digis in and etsums out

Removed a few leftover items causing compile failure from last stripdown of superfluous elements - need to fix the BuildFile.xml still

Added I think the only include statement needed for QIE10DataFrame

Added a test file l1ZDCProducerTest.py based on GM work here: https://github.com/ginnocen/UPCopenHFanalysis/tree/zdc_calibrationcode/zdc_calibration/newZDCAnalyzer/test

Initial copy of L1TStage2CaloAnlyzer to L1TZDCAnalyzer, just renaming the class, no internal changes (next commit attempt to strip down to etsums only)

Updated to a working producer that outputs dummy etsums and an analyzers that compiles, building a simple ttree for etsums per event - next step is to get the python running interfacing producer with analyzer, and replacing dummy et sums

Fixed both the L1TZDCAnalyzer for not filling the TTree, moved ET in names to Et, and fixed issues in the l1ZDCProducerTest python for not picking up the ETSums

Forgot to remove the bx checks from the last push in L1ZDCAnalyzer - remove for now but be ready to revert if this is important to L1 folks

Fixed two small errors in the L1TZDCAnalyzer and L1TZDCProducer, and added a README w/ build instructions under L1Trigger/L1TZDC

Fixed mistake in README

Updated the L1TZDCAnalyzer, Producer, and ProducerTest.py files under L1TZDC via ginnocen edits that can be found here: ginnocen@1554043 + the previous commit; in future we will try to switch to doing this properly by pull requests

Last build was successful which means we can increment to v0.1.0. Version up because working version with ZDCEtSum output, non-dummy. Next increment to v0.2.0 with lookup tables instead of hard-coded LUT and compatibility with CMSSW_13_2_0_pre3. Also modified README to reflect the latest build

First pass at adding ZDC LUT handling to CaloParamsHelper.h, L1TCaloStage2ParamsESProducer.cc, and caloParams_cfi.py

Updated for the changes ginnocen made in this commit ginnocen@5b8bc71 along with updated README instructions to tag v0.1.3 this will corresppond to - again, need to switch to proper PR system over linking git commits hack

Updated DataFormats/L1TGlobal/*/GlobalObject.* files for gtZDCP and gtZDCM - also updated the ZDC Producer to match the P/M convention for Plus/Minus rather than our existing choice of P/N for Positive/Negative

Fixed bug in l1ZDCProducerTest.py (left input as zdcEtSumsN instead of zdcEtSumsM, change to match convention), updated EtSum for kZDCP and kZDCM, updated L1TZDCProducer to use this new type, and readme to use this tag=zdcL1T_v0.1.5

Updating the L1TNtuples for a temp ZDCP and ZDCM etSums (not using the enum position of this etsum but rather as independent objects for speedy implementation

Modified caloParams so the LUTFile isnt empty and updated the README for latest build instructions getting the ZDCSums into the L1TNtuple

Large update to the ZDCProducer replacing hard-coded lut w/ py lut (toggleable for now); Analyzer fix (all bx except 4 off, plus N -> M in ZDC naming convention); createLUT for flattened file; l1ZDCProducerTest and a data dir for the python luts under L1TZDC

Small fixes for LUT + reduced bx from 10 to 5 (centered on bx 4, position of peak, for 2, 3, 4, 5, 6 in zdc output) - Analyzer nBX reduced to 5 (hardcoded), also LUT in caloParams now points to what we pull in under L1Trigger/L1TZDC/data - this is the set of changes for incrementing to v0.2.0

Updated README file to fix readability and add some common debug info

Still fixing README

Updated README for more info that was missing in build + fixed a bunch of typos

Updated README for git tag zdcL1T_latest to avoid future need for README updates

Collapsed ZDCP and ZDCM outputs of the L1TZDCProducer into a single ETSum; P and M now distinguished by kZDCP and kZDCM EtSum type id defined in the data format - also altered the test L1TZDCAnalyzer and the l1ZDCProducerTest.py file, checked the outputs looked unchanged when running with this update

Mass update for Upgrade tree and update to README for clearer instructions + modified for combining ZDC EtSums into one object

Commit after testing of zdcL1T branch for PR; tested on 2023.08.22 w/ CMSSW_13_3_x_2023-08-22-1100 i.e. latest daily. Two tests fail but inspecting log shows files were not accessible (even after double checking voms was done) - will request advice on PR

Updated files per initial comments on PR here: https://github.com/cms-sw/cmssw/pull/42634/files ; also attempted to fix in additional places as yet uncommented with similar issues

Fixes for code format issues flagged in git cmssw bot checks on last commit; just ran scram build code-format on the command line

Modifications for PR; mostly removal of comments no longer relevant, removal of functions not relevant, switching from cout to LogInfo, and switching to unique_ptr for handling caloParamsHelper

Removed unused iterator called counter from Producer, and changed a cout to edm::LogInfo inAnalyzer

Merging in PR from elfontan, cms-sw#42635, handling of the uGT side of the ZDC trigger; ran scram runtests, codechecks and code format before push; some change to L1TZDC code to fix residual differences between prs
@fwyzard
Copy link
Contributor

fwyzard commented Aug 29, 2023

@elfontan I understand this PR is now part of #42634.
Could you close it to avoid warning messages from @cmsbuild like these ones ?

@elfontan elfontan closed this Aug 29, 2023
@elfontan
Copy link
Contributor Author

Done, thank you @fwyzard!

cfmcginn added a commit to cfmcginn/cmssw that referenced this pull request Aug 31, 2023
…after L1TCalorimeter

Updated naming scheme to remove Stage2 in the ZDC producer - also added contact info and dates in event of bugs/history issues

Stripped out everything except for digis in and etsums out

Removed a few leftover items causing compile failure from last stripdown of superfluous elements - need to fix the BuildFile.xml still

Added I think the only include statement needed for QIE10DataFrame

Added a test file l1ZDCProducerTest.py based on GM work here: https://github.com/ginnocen/UPCopenHFanalysis/tree/zdc_calibrationcode/zdc_calibration/newZDCAnalyzer/test

Initial copy of L1TStage2CaloAnlyzer to L1TZDCAnalyzer, just renaming the class, no internal changes (next commit attempt to strip down to etsums only)

Updated to a working producer that outputs dummy etsums and an analyzers that compiles, building a simple ttree for etsums per event - next step is to get the python running interfacing producer with analyzer, and replacing dummy et sums

Fixed both the L1TZDCAnalyzer for not filling the TTree, moved ET in names to Et, and fixed issues in the l1ZDCProducerTest python for not picking up the ETSums

Forgot to remove the bx checks from the last push in L1ZDCAnalyzer - remove for now but be ready to revert if this is important to L1 folks

Fixed two small errors in the L1TZDCAnalyzer and L1TZDCProducer, and added a README w/ build instructions under L1Trigger/L1TZDC

Fixed mistake in README

Updated the L1TZDCAnalyzer, Producer, and ProducerTest.py files under L1TZDC via ginnocen edits that can be found here: ginnocen@1554043 + the previous commit; in future we will try to switch to doing this properly by pull requests

Last build was successful which means we can increment to v0.1.0. Version up because working version with ZDCEtSum output, non-dummy. Next increment to v0.2.0 with lookup tables instead of hard-coded LUT and compatibility with CMSSW_13_2_0_pre3. Also modified README to reflect the latest build

First pass at adding ZDC LUT handling to CaloParamsHelper.h, L1TCaloStage2ParamsESProducer.cc, and caloParams_cfi.py

Updated for the changes ginnocen made in this commit ginnocen@5b8bc71 along with updated README instructions to tag v0.1.3 this will corresppond to - again, need to switch to proper PR system over linking git commits hack

Updated DataFormats/L1TGlobal/*/GlobalObject.* files for gtZDCP and gtZDCM - also updated the ZDC Producer to match the P/M convention for Plus/Minus rather than our existing choice of P/N for Positive/Negative

Fixed bug in l1ZDCProducerTest.py (left input as zdcEtSumsN instead of zdcEtSumsM, change to match convention), updated EtSum for kZDCP and kZDCM, updated L1TZDCProducer to use this new type, and readme to use this tag=zdcL1T_v0.1.5

Updating the L1TNtuples for a temp ZDCP and ZDCM etSums (not using the enum position of this etsum but rather as independent objects for speedy implementation

Modified caloParams so the LUTFile isnt empty and updated the README for latest build instructions getting the ZDCSums into the L1TNtuple

Large update to the ZDCProducer replacing hard-coded lut w/ py lut (toggleable for now); Analyzer fix (all bx except 4 off, plus N -> M in ZDC naming convention); createLUT for flattened file; l1ZDCProducerTest and a data dir for the python luts under L1TZDC

Small fixes for LUT + reduced bx from 10 to 5 (centered on bx 4, position of peak, for 2, 3, 4, 5, 6 in zdc output) - Analyzer nBX reduced to 5 (hardcoded), also LUT in caloParams now points to what we pull in under L1Trigger/L1TZDC/data - this is the set of changes for incrementing to v0.2.0

Updated README file to fix readability and add some common debug info

Still fixing README

Updated README for more info that was missing in build + fixed a bunch of typos

Updated README for git tag zdcL1T_latest to avoid future need for README updates

Collapsed ZDCP and ZDCM outputs of the L1TZDCProducer into a single ETSum; P and M now distinguished by kZDCP and kZDCM EtSum type id defined in the data format - also altered the test L1TZDCAnalyzer and the l1ZDCProducerTest.py file, checked the outputs looked unchanged when running with this update

Mass update for Upgrade tree and update to README for clearer instructions + modified for combining ZDC EtSums into one object

Commit after testing of zdcL1T branch for PR; tested on 2023.08.22 w/ CMSSW_13_3_x_2023-08-22-1100 i.e. latest daily. Two tests fail but inspecting log shows files were not accessible (even after double checking voms was done) - will request advice on PR

Updated files per initial comments on PR here: https://github.com/cms-sw/cmssw/pull/42634/files ; also attempted to fix in additional places as yet uncommented with similar issues

Fixes for code format issues flagged in git cmssw bot checks on last commit; just ran scram build code-format on the command line

Modifications for PR; mostly removal of comments no longer relevant, removal of functions not relevant, switching from cout to LogInfo, and switching to unique_ptr for handling caloParamsHelper

Removed unused iterator called counter from Producer, and changed a cout to edm::LogInfo inAnalyzer

Merging in PR from elfontan, cms-sw#42635, handling of the uGT side of the ZDC trigger; ran scram runtests, codechecks and code format before push; some change to L1TZDC code to fix residual differences between prs

Fix from @elfontan for Zdc ESum Condition replacing existing ESum Condition - now passes test in testL1TGlobalProducer.sh - also added requested protections for L1TZDCProducer - check if input is valid and if no, output empty collection with a warning
missirol pushed a commit to missirol/cmssw that referenced this pull request Sep 2, 2023
…after L1TCalorimeter

Updated naming scheme to remove Stage2 in the ZDC producer - also added contact info and dates in event of bugs/history issues

Stripped out everything except for digis in and etsums out

Removed a few leftover items causing compile failure from last stripdown of superfluous elements - need to fix the BuildFile.xml still

Added I think the only include statement needed for QIE10DataFrame

Added a test file l1ZDCProducerTest.py based on GM work here: https://github.com/ginnocen/UPCopenHFanalysis/tree/zdc_calibrationcode/zdc_calibration/newZDCAnalyzer/test

Initial copy of L1TStage2CaloAnlyzer to L1TZDCAnalyzer, just renaming the class, no internal changes (next commit attempt to strip down to etsums only)

Updated to a working producer that outputs dummy etsums and an analyzers that compiles, building a simple ttree for etsums per event - next step is to get the python running interfacing producer with analyzer, and replacing dummy et sums

Fixed both the L1TZDCAnalyzer for not filling the TTree, moved ET in names to Et, and fixed issues in the l1ZDCProducerTest python for not picking up the ETSums

Forgot to remove the bx checks from the last push in L1ZDCAnalyzer - remove for now but be ready to revert if this is important to L1 folks

Fixed two small errors in the L1TZDCAnalyzer and L1TZDCProducer, and added a README w/ build instructions under L1Trigger/L1TZDC

Fixed mistake in README

Updated the L1TZDCAnalyzer, Producer, and ProducerTest.py files under L1TZDC via ginnocen edits that can be found here: ginnocen@1554043 + the previous commit; in future we will try to switch to doing this properly by pull requests

Last build was successful which means we can increment to v0.1.0. Version up because working version with ZDCEtSum output, non-dummy. Next increment to v0.2.0 with lookup tables instead of hard-coded LUT and compatibility with CMSSW_13_2_0_pre3. Also modified README to reflect the latest build

First pass at adding ZDC LUT handling to CaloParamsHelper.h, L1TCaloStage2ParamsESProducer.cc, and caloParams_cfi.py

Updated for the changes ginnocen made in this commit ginnocen@5b8bc71 along with updated README instructions to tag v0.1.3 this will corresppond to - again, need to switch to proper PR system over linking git commits hack

Updated DataFormats/L1TGlobal/*/GlobalObject.* files for gtZDCP and gtZDCM - also updated the ZDC Producer to match the P/M convention for Plus/Minus rather than our existing choice of P/N for Positive/Negative

Fixed bug in l1ZDCProducerTest.py (left input as zdcEtSumsN instead of zdcEtSumsM, change to match convention), updated EtSum for kZDCP and kZDCM, updated L1TZDCProducer to use this new type, and readme to use this tag=zdcL1T_v0.1.5

Updating the L1TNtuples for a temp ZDCP and ZDCM etSums (not using the enum position of this etsum but rather as independent objects for speedy implementation

Modified caloParams so the LUTFile isnt empty and updated the README for latest build instructions getting the ZDCSums into the L1TNtuple

Large update to the ZDCProducer replacing hard-coded lut w/ py lut (toggleable for now); Analyzer fix (all bx except 4 off, plus N -> M in ZDC naming convention); createLUT for flattened file; l1ZDCProducerTest and a data dir for the python luts under L1TZDC

Small fixes for LUT + reduced bx from 10 to 5 (centered on bx 4, position of peak, for 2, 3, 4, 5, 6 in zdc output) - Analyzer nBX reduced to 5 (hardcoded), also LUT in caloParams now points to what we pull in under L1Trigger/L1TZDC/data - this is the set of changes for incrementing to v0.2.0

Updated README file to fix readability and add some common debug info

Still fixing README

Updated README for more info that was missing in build + fixed a bunch of typos

Updated README for git tag zdcL1T_latest to avoid future need for README updates

Collapsed ZDCP and ZDCM outputs of the L1TZDCProducer into a single ETSum; P and M now distinguished by kZDCP and kZDCM EtSum type id defined in the data format - also altered the test L1TZDCAnalyzer and the l1ZDCProducerTest.py file, checked the outputs looked unchanged when running with this update

Mass update for Upgrade tree and update to README for clearer instructions + modified for combining ZDC EtSums into one object

Commit after testing of zdcL1T branch for PR; tested on 2023.08.22 w/ CMSSW_13_3_x_2023-08-22-1100 i.e. latest daily. Two tests fail but inspecting log shows files were not accessible (even after double checking voms was done) - will request advice on PR

Updated files per initial comments on PR here: https://github.com/cms-sw/cmssw/pull/42634/files ; also attempted to fix in additional places as yet uncommented with similar issues

Fixes for code format issues flagged in git cmssw bot checks on last commit; just ran scram build code-format on the command line

Modifications for PR; mostly removal of comments no longer relevant, removal of functions not relevant, switching from cout to LogInfo, and switching to unique_ptr for handling caloParamsHelper

Removed unused iterator called counter from Producer, and changed a cout to edm::LogInfo inAnalyzer

Merging in PR from elfontan, cms-sw#42635, handling of the uGT side of the ZDC trigger; ran scram runtests, codechecks and code format before push; some change to L1TZDC code to fix residual differences between prs
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.

7 participants