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

finecalo: No longer count reentries into the calorimeter as boundary crossings #34599

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

tklijnsma
Copy link

PR description:

No longer counts reentries into the calorimeter as boundary crossings. The boundary-crossing position and momentum for tracks that started in the calorimeter or entered before are no longer (over)written.

PR validation:

Verified with debug statements in the code.

@lgray
Copy link
Contributor

lgray commented Jul 22, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34599/24159

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @tklijnsma for master.

It involves the following packages:

  • SimG4CMS/Calo (simulation)
  • SimG4Core/Notification (simulation)

@civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @cvuosalo, @rovere, @bsunanda, @thomreis, @felicepantaleo, @simonepigazzini, @fabiocos, @slomeo this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fa2d19/17112/summary.html
COMMIT: c931372
CMSSW: CMSSW_12_0_X_2021-07-22-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34599/17112/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 22-Jul-2021 21:58:01 CEST-----------------------
An exception of category 'StdException' occurred while
   [0] Running EventSetup component DDDetectorESProducer/'
Exception Message:
A std::exception was thrown.
An exception of category 'FileInPathError' occurred.
Exception Message:
edm::FileInPath unable to find file Geometry/VeryForwardData/data/RP_Vertical_Device/2021/v1/RP_Vertical_Device.xml anywhere in the search path.
The search path is defined by: CMSSW_SEARCH_PATH
${CMSSW_SEARCH_PATH} is: /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34599/17112/CMSSW_12_0_X_2021-07-22-1100/poison:/cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34599/17112/CMSSW_12_0_X_2021-07-22-1100/src:/cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34599/17112/CMSSW_12_0_X_2021-07-22-1100/external/slc7_amd64_gcc900/data:/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc900/cms/cmssw/CMSSW_12_0_X_2021-07-22-1100/src:/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc900/cms/cmssw/CMSSW_12_0_X_2021-07-22-1100/external/slc7_amd64_gcc900/data
Current directory is: /data/cmsbld/jenkins/workspace/ib-run-pr-relvals/runTheMatrix-results/11634.911_TTbar_14TeV+2021_DD4hep+TTbar_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST+ALCA

dd4hep: Error interpreting XML nodes of type <Include/>
dd4hep: Error interpreting XML nodes of type <IncludeSection/>
dd4hep: while parsing /cvmfs/cms-ib.cern.ch/nweek-02690/slc7_amd64_gcc900/cms/cmssw/CMSSW_12_0_X_2021-07-22-1100/src/Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021.xml
dd4hep: with plugin:DD4hep_CompactLoader
----- End Fatal Exception -------------------------------------------------

@perrotta
Copy link
Contributor

@cmsbuild please test

1 similar comment
@lgray
Copy link
Contributor

lgray commented Jul 28, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fa2d19/17318/summary.html
COMMIT: c931372
CMSSW: CMSSW_12_0_X_2021-07-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34599/17318/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2998564
  • DQMHistoTests: Total failures: 13
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2998529
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Jul 29, 2021

+1

@perrotta
Copy link
Contributor

perrotta commented Aug 2, 2021

@tklijnsma are we learning anything with these tests with the fineCalo procModifier enabled?
Can you comment on the test failures?

@civanch
Copy link
Contributor

civanch commented Aug 2, 2021

11634.911 and 11634.912 WFs are for DD4Hep, I suspect that modifier does not applicable in a simple way. I would merge this and another PRs for finecalo, because it pass ordinary set of tests and not affect mainstream. In fact, finecalo is a special option needed for development.

@perrotta
Copy link
Contributor

perrotta commented Aug 2, 2021

+1

  • Simple addition
  • Tests succeed with no changes in output on standard workflows
  • '--procModifiers fineCalo' will be selected in dedicated workflows, now under development

@srimanob
Copy link
Contributor

srimanob commented Aug 2, 2021

@perrotta , please note that --command '--procModifiers fineCalo' will override --command '--customise Validation/Performance/TimeMemoryJobReport.customiseWithTimeMemoryJobReport' option ( https://github.com/cms-sw/cms-bot/blob/master/pr_testing/test_multiple_prs.sh#L1002-L1005 ). In order to let runTheMatrix use both options we need to update https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/scripts/runTheMatrix.py#L229-L232 and added action="append",

I make a PR in
#34722
not sure it is all we need. @smuzaffar @perrotta

@srimanob
Copy link
Contributor

srimanob commented Aug 2, 2021

+Upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 2, 2021

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing).

@perrotta
Copy link
Contributor

perrotta commented Aug 2, 2021

merge

@srimanob
Copy link
Contributor

srimanob commented Aug 2, 2021

@civanch
Should we disable it from https://github.com/cms-sw/cmssw/blob/master/SimG4Core/Application/python/g4SimHits_cfi.py#L65-L69 first?

The error also shows in reminiaod data workflow also, 136.88811.

@civanch
Copy link
Contributor

civanch commented Aug 3, 2021

@srimanob, I am not sure if we should disable modifier inside g4SimHits_cfi. Also I am not sure if modifier for this option is the best solution. I personally prefer customization for such parameters. Inside g4SimHits_cfi there are many parameters, the modifier for these HGCal parameters should be re-discussed.

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