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

Adding HGCAL simhit validation histograms #36484

Merged
merged 3 commits into from Dec 18, 2021

Conversation

indra-ehep
Copy link
Contributor

PR description:

PR validation:

All the suggested tests are perfomed as mentioned in https://cms-sw.github.io/PRWorkflow.html including 'scram build code-checks' and 'scram build code-format'

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not applicable.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36484/27356

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @indra-ehep for master.

It involves the following packages:

  • Validation/HGCalValidation (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@vandreev11, @sethzenz, @bsunanda, @rovere, @lgray, @cseez, @apsallid, @pfs, @lecriste, @hatakeyamak, @ebrondol this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

@indra-ehep can you please add yourself along with your github username in the comments filed to the HGCAL Validation developers e-group to keep track of the people working on it?
https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMContacts#HGCAL
Thanks

@indra-ehep
Copy link
Contributor Author

@indra-ehep can you please add yourself along with your github username in the comments filed to the HGCAL Validation developers e-group to keep track of the people working on it? https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMContacts#HGCAL Thanks

Dear Javier,
Thank you for prompt response. I have added myself to the list that you have mentioned. Now waiting for approval.
regards
Indra
Screenshot at 2021-12-14 17-18-11

@jfernan2
Copy link
Contributor

please test

@indra-ehep
Copy link
Contributor Author

please test

Thank you. I see that I am a member now.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: Build HeaderConsistency ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44816f/21251/summary.html
COMMIT: 67b311f
CMSSW: CMSSW_12_3_X_2021-12-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36484/21251/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44816f/21251/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44816f/21251/git-merge-result

Build

I found compilation error when building:

/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/functors/slot.h:1655:7:   required from 'class sigc::slot'
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/signal.h:658:41:   required from 'struct sigc::internal::signal_emit0'
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/signal.h:2635:54:   required from 'class sigc::signal0'
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/signal.h:3839:7:   required from 'class sigc::signal'
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2021-12-13-1100/src/Fireworks/Core/interface/FWColorManager.h:101:32:   required from here
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/functors/slot.h:431:22: error: function returning a function
  431 |   typedef T_return (*call_type)(rep_type*);
      |                      ^~~~~~~~~
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/functors/slot.h:437:19: error: function returning a function
  437 |   inline T_return operator()() const
      |                   ^~~~~~~~


Clang Build

I found compilation error 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'

>> Entering Package RecoEgamma/EgammaTools
>> Entering Package RecoVertex/BeamSpotProducer
>> Entering Package SimTransport/PPSProtonTransport
>> Entering Package Validation/HGCalValidation
>> Compile sequence completed for CMSSW CMSSW_12_3_X_2021-12-13-1100
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 1
+ eval scram build outputlog '&&' '(python3' /data/cmsbld/jenkins/workspace/ib-run-pr-tests/cms-bot/buildLogAnalyzer.py --ignoreWarning=Wdeprecated-declarations --logDir /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2021-12-13-1100/tmp/slc7_amd64_gcc900/cache/log/src '||' 'true)'
++ scram build outputlog
>> Entering Package Alignment/OfflineValidation
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2021-12-13-1100/src/Alignment/OfflineValidation/bin/DMRtrends.cc
Entering library rule at src/Alignment/OfflineValidation/plugins


@perrotta
Copy link
Contributor

please test
(CMSSW_12_3_X_2021-12-13-2300, now available, contains the needed external)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44816f/21259/summary.html
COMMIT: 67b311f
CMSSW: CMSSW_12_3_X_2021-12-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36484/21259/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250704
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3250682
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 2203098.4 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 445730.602 KiB HGCAL/HGCalSimHitsV
  • DQMHistoSizes: changed ( 38634.0 ): 420175.992 KiB HGCAL/HGCalSimHitsV
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

I am sorry @indra-ehep but your PR is adding more than 400MB of new MEs to the DQM root file.
You should reduce the memory footprint, I suggest reducing the number of MEs and/or the binning (1k bins in 1D and 600x600 in 2D plots is too much)
Please inspect the results and reduce accordingly:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2021-12-13-2300+44816f/47470/dqm-histo-comparison-summary.html
Thanks

@indra-ehep
Copy link
Contributor Author

I am sorry @indra-ehep but your PR is adding more than 400MB of new MEs to the DQM root file. You should reduce the memory footprint, I suggest reducing the number of MEs and/or the binning (1k bins in 1D and 600x600 in 2D plots is too much) Please inspect the results and reduce accordingly: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2021-12-13-2300+44816f/47470/dqm-histo-comparison-summary.html Thanks

Dear Javier,

The main issue of large memory footprint is due to TH2. Earlier we have used TGraph instead of TH2 which contains the same information but with a significantly smaller size. However, I did not find any handler for TGraph in DQMStore.h. May be it is mentioned in other files that I missed. So how do we proceed now ?

Plan A : I can switch off the TH2 for now, re-optimize binning of 1D histograms. Then commit the changes. Later we add the TGraph features when available in DQMStore.h after contacting DQM experts.

Plan B : I add TGraph feature in DQMStore.h and then modify HGCalSimHitValidation.cc for TGraph and 1D histograms.

Or if you have other plan. Please let me know your feedback.

regards
Indra

@indra-ehep
Copy link
Contributor Author

I am sorry @indra-ehep but your PR is adding more than 400MB of new MEs to the DQM root file. You should reduce the memory footprint, I suggest reducing the number of MEs and/or the binning (1k bins in 1D and 600x600 in 2D plots is too much) Please inspect the results and reduce accordingly: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2021-12-13-2300+44816f/47470/dqm-histo-comparison-summary.html Thanks

Dear Javier,
Sorry for multiple posts. Is there any tool in CMSSW to check how much extra storage requirement is added in DQM.root file due to the new changes ? Then I can run such tests locally before making the git push.
regards
Indra

@indra-ehep
Copy link
Contributor Author

@jfernan2 I have pushed the file that has been agreed. Thanks.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36484/27446

  • This PR adds an extra 20KB to repository

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

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

Pull request #36484 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please check and sign again.

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44816f/21365/summary.html
COMMIT: 5993f50
CMSSW: CMSSW_12_3_X_2021-12-17-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36484/21365/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250719
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3250691
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 33718.237 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 23234.0,... ): 6818.627 KiB HGCAL/HGCalSimHitsV
  • DQMHistoSizes: changed ( 38634.0 ): 6443.729 KiB HGCAL/HGCalSimHitsV
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

  • The size increase of the DQM root files, all below 7 MB, is deemed acceptable by DQM

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.

None yet

6 participants