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

Phase2-hgx326X Try to address cassette shift abnormality for HGCal geometry #39625

Closed
wants to merge 6 commits into from

Conversation

bsunanda
Copy link
Contributor

@bsunanda bsunanda commented Oct 5, 2022

PR description:

Try to address cassette shift abnormality for HGCal geometry

PR validation:

Use the runTheMatrix test workflows

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Nothing special

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39625/32419

  • This PR adds an extra 80KB to repository

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39625/32420

  • This PR adds an extra 80KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

  • Geometry/HGCalCommonData (geometry, upgrade)
  • SimG4CMS/Calo (simulation)
  • Validation/HGCalValidation (dqm)

@civanch, @Dr15Jones, @bsunanda, @makortel, @emanueleusai, @ianna, @ahmad3213, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @mdhildreth, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@youyingli, @vandreev11, @fabiocos, @lecriste, @sethzenz, @missirol, @felicepantaleo, @rovere, @lgray, @cseez, @apsallid, @pfs, @thomreis, @hatakeyamak, @trtomei, @ebrondol, @beaucero, @slomeo, @simonepigazzini this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39625/32422

  • This PR adds an extra 88KB to repository

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39625/32423

  • This PR adds an extra 88KB to repository

@bsunanda
Copy link
Contributor Author

bsunanda commented Oct 9, 2022

@cmsbuild Please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 9, 2022

-1

Failed Tests: RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b8ded7/28132/summary.html
COMMIT: 002f635
CMSSW: CMSSW_12_6_X_2022-10-08-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39625/28132/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 09-Oct-2022 06:28:40 CEST-----------------------
An exception of category 'OutOfBound' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 1 stream: 0
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module L1TEGMultiMerger/'l1tLayer1EG'
   [4] Prefetching for module L1TCorrelatorLayer1Producer/'l1tLayer1HGCal'
   [5] Prefetching for module PFClusterProducerFromHGC3DClusters/'l1tPFClustersFromHGC3DClusters'
   [6] Calling method for module HGCalBackendLayer2Producer/'l1tHGCalBackEndLayer2Producer'
Exception Message:
TC X1 = 3.18723 out of the seeding histogram bounds 0.076 - 0.58
----- End Fatal Exception -------------------------------------------------

RelVals-INPUT

  • 39434.10339434.103_TTbar_14TeV+2026D88Aging3000+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+DigiTrigger+RecoGlobal+HARVESTGlobal/step2_TTbar_14TeV+2026D88Aging3000+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+DigiTrigger+RecoGlobal+HARVESTGlobal.log
  • 39634.11439634.114_TTbar_14TeV+2026D88PU_OTInefficiency10PC+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU/step2_TTbar_14TeV+2026D88PU_OTInefficiency10PC+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU.log

@bsunanda
Copy link
Contributor Author

bsunanda commented Oct 9, 2022

@perrotta There were several serious bugs in the code which is corrected here. So if any external file is used in the testing, they are needed to be recreated for the tests. Can the tests be avoided?

@perrotta
Copy link
Contributor

perrotta commented Oct 9, 2022

@perrotta There were several serious bugs in the code which is corrected here. So if any external file is used in the testing, they are needed to be recreated for the tests. Can the tests be avoided?

@bsunanda this PR must be eventually merged in the master, i.e. it has to run without crashing in it. Please provide all what is needed to run without errors, and test it all together: there is no point in merging a PR that is known to crash once merged.

@bsunanda
Copy link
Contributor Author

bsunanda commented Oct 9, 2022

@perrotta My point was that if the additional files which are needed in the test be created with the same PR

@perrotta
Copy link
Contributor

perrotta commented Oct 9, 2022

@perrotta My point was that if the additional files which are needed in the test be created with the same PR

Please create those files and add them as externals, if this is what is needed.
If you intended something different, please tell explicitely what is missing here to run without crashing and how to produce it in practice.

@srimanob
Copy link
Contributor

@perrotta There were several serious bugs in the code which is corrected here. So if any external file is used in the testing, they are needed to be recreated for the tests. Can the tests be avoided?

Hi @bsunanda @perrotta
Sorry to jump in. May I clarify on the above statement? Do you mean

  • external as mentioned by @perrotta
  • or you mean to recreate MinBias that use for mixing? The failure appears in the workflows that use existing MinBias. Workflow .999 use MinBias to create pre-mixing file.

Actually, I don't understand the failure from PR test. It seems the failure happens because of L1T, but why it fails in D88 which I assume you don't want to touch it. I assume from the PR title (idea of cassette) that it will affect v17 HGCAL only, not v16.

As usual request, would you mind to make a PR with detail in the PR description? If this issue is presented somewhere, link will be useful. Currently, it does not tell us anything on what we expect, what we should do to move this on.

Thanks.

@indra-ehep
Copy link
Contributor

@perrotta , @salvatore @smuzaffar : Can you please share the workflow number corresponding to MinBias event creation ? We
want to use that to validate the current PR.

@srimanob
Copy link
Contributor

Hi @indra-ehep

Here is the workflow that you can use to produce MinBias GS:
39440.0 2026D88+MinBias_14TeV_pythia8_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal
41040.0 2026D92+MinBias_14TeV_pythia8_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

To check the workflow ID, you can i.e.
runTheMatrix.py --what upgrade -n | grep D88 | grep MinBias | grep 14TeV | sed /PU/d

@rovere
Copy link
Contributor

rovere commented Oct 11, 2022

Ciao all,
IIUC, the crash could be due to the fact that the PU library used for testing does not include the fixes that are contained in this PR. It would be nice to understand what those fixes are and why the fixes as proposed in this PR will cause the crash.
@jbsauvan FYI: I'd rather L1 HGCAL people be involved in this and, eventually, bless this PR or at least be made aware of the problems that were there and that are not gone.
On a final note, the merging of this PR will inevitably cause failures in the testing of all other PRs (again, IIUC what the origin of the failures is), unless the minBias sample is regenerated, which would require a release with this PR merged... Quite a catch-22 situation.
Release managers, if that's the situation, do you have any recommendations on how to proceed?

@rovere
Copy link
Contributor

rovere commented Oct 11, 2022

assign hgcal-dpg

@cmsbuild
Copy link
Contributor

New categories assigned: hgcal-dpg

@felicepantaleo,@rovere,@pfs,@cseez you have been requested to review this Pull request/Issue and eventually sign? Thanks

@jbsauvan
Copy link
Contributor

Hello,
The following exception

Exception Message:
TC X1 = 3.18723 out of the seeding histogram bounds 0.076 - 0.58

means that there are trigger cells (and therefore detector cells?) well outside the typical HGCAL acceptance (3.19 >> 0.58 which was about the maximum observed so far). X1 here is r/z where r is the radial distance from the beam.
We could make a change in the TPG code to avoid this exception. But is this expected to have cells with r/z>3?

@bsunanda
Copy link
Contributor Author

bsunanda commented Oct 11, 2022 via email

@indra-ehep
Copy link
Contributor

Hi @indra-ehep

Here is the workflow that you can use to produce MinBias GS: 39440.0 2026D88+MinBias_14TeV_pythia8_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal 41040.0 2026D92+MinBias_14TeV_pythia8_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

To check the workflow ID, you can i.e. runTheMatrix.py --what upgrade -n | grep D88 | grep MinBias | grep 14TeV | sed /PU/d

Thank you @srimanob

@jbsauvan
Copy link
Contributor

Could you provide the DetId which provides this value and how this ratio is calculated in the code?

Hi @bsunanda

  • The calculation of r/z is done here
  • I have put here an example showing how to get the TC detid

@bsunanda
Copy link
Contributor Author

Made a new PR#40404 with correct specification. So close this unwanted PR

@bsunanda bsunanda closed this Dec 25, 2022
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