Skip to content

Implement TH2Poly in DQM Services for HGCal DQM #41932

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

Merged
merged 13 commits into from
Mar 10, 2025

Conversation

ywkao
Copy link
Contributor

@ywkao ywkao commented Jun 12, 2023

PR description:

This PR introduces a new type of DQM MonitorElement, TH2Poly, for HGCal DQM in the future. This feature allows a display of polygonal histograms on the CMS DQM GUI. As a demonstration, a wafer map can be displayed like the screenshot here [1].

TH2Poly is a 2D histogram class inherited from TH2. Polygonal bins, defined by TGraph, can be loaded using the AddBin() method. After setting up the polygonal bins, a TH2Poly object can store information through Fill() or SetBinContent().

A workflow for creating polygonal histograms looks like this:
DQM Service -> DQM EDAnalyzer -> CMS DQM GUI

An implementation of TH2Poly in DQM Service and MonitorElement is necessary to display the polygonal histograms. It involves updates on two repositories: cmssw and dqmgui_prod. A pull request is created in the dqmgui_prod repository [2] with a relevant issue reported in this link [3], which is about setting up a CMS DQM GUI with the new feature.

PR validation:

The workflow and the implementation have been tested: (a) From this feature branch, monitor elements of TH2Poly can be stored in a DQM root file [4]. (b) The DQM root file can be uploaded to a CMS DQM GUI, which is built following the steps noted in this issue [3]. Polygonal maps can be displayed on the DQM GUI, as demonstrated in [1].

[1] https://ykao.web.cern.ch/ykao/raw_data_handling/hgcal_dqm_gui/screenshot_demo_th2poly_wafermap.png
[2] cms-DQM/dqmgui_prod#14
[3] cms-DQM/dqmgui_prod#13
[4] A DQM root file containing demo polygonal maps: /afs/cern.ch/work/y/ykao/public/example_HGCAL_DQM/DQM_V0001_HGCAL_R000123469.root

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41932/35885

  • This PR adds an extra 44KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 12, 2023

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

It involves the following packages:

  • DQMServices/Core (dqm)
  • DataFormats/Histograms (dqm, core)

@smuzaffar, @Dr15Jones, @makortel, @nothingface0, @emanueleusai, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@missirol, @barvic, @rovere 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

Copy link
Contributor

@makortel makortel left a comment

Choose a reason for hiding this comment

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

I think in addition the following places need to be amended accordingly

static TreeHelperBase* makeHelper(unsigned int iTypeIndex, TTree* iTree, std::string* iFullNameBufferPtr) {
switch (iTypeIndex) {
case kIntIndex:
return new IntTreeHelper(iTree, iFullNameBufferPtr);

m_treeReaders[kTProfileIndex].reset(new TreeObjectReader<TProfile>(MonitorElementData::Kind::TPROFILE, m_rescope));
m_treeReaders[kTProfile2DIndex].reset(
new TreeObjectReader<TProfile2D>(MonitorElementData::Kind::TPROFILE2D, m_rescope));
}

How about adding some tests to make sure all the I/O components work properly? (I don't remember if there are already any such tests that could be easily amended)

double lowY,
double highY,
FUNC onbooking = NOOP()) {
return bookME(name, MonitorElementData::Kind::TH2F, [=]() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
return bookME(name, MonitorElementData::Kind::TH2F, [=]() {
return bookME(name, MonitorElementData::Kind::TH2Poly, [=]() {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this should be corrected. I will update it in a commit accordingly.

/// set polygon bin (TH2Poly)
void MonitorElement::addBin(TGraph *graph) {
auto access = this->accessMut();
static_cast<TH2Poly *>(accessRootObject(access, __PRETTY_FUNCTION__, 2))->AddBin(graph);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the ROOT object is not TH2Poly? The other similar functions throw an exception via the incompatible() function.

Copy link
Contributor Author

@ywkao ywkao Jun 13, 2023

Choose a reason for hiding this comment

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

Indeed, incompatible() function is necessary. This block of code will be modified as follows:

-    static_cast<TH2Poly *>(accessRootObject(access, __PRETTY_FUNCTION__, 2))->AddBin(graph);
+    if (kind() == Kind::TH2Poly) {
+      static_cast<TH2Poly *>(accessRootObject(access, __PRETTY_FUNCTION__, 2))->AddBin(graph);
+    } else {
+      incompatible(__PRETTY_FUNCTION__);
+    }

@@ -140,7 +140,8 @@ struct MonitorElementData {
TH2I = 0x23,
TH3F = 0x30,
TPROFILE = 0x40,
TPROFILE2D = 0x41
TPROFILE2D = 0x41,
TH2Poly = 0x60
Copy link
Contributor

Choose a reason for hiding this comment

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

Why such a large gap between the previous value (0x41)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I kept thinking of hexagons for a wafer map and then put a number starting with six. We can assign 0x24 for TH2Poly if it is feasible.

+    TH2Poly = 0x24,
     TH3F = 0x30,
     TPROFILE = 0x40,
-    TPROFILE2D = 0x41,
-    TH2Poly = 0x60
+    TPROFILE2D = 0x41

@emanueleusai
Copy link
Member

type hgcal

@emanueleusai
Copy link
Member

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-03d614/33112/summary.html
COMMIT: 5fa5e4e
CMSSW: CMSSW_13_2_X_2023-06-12-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41932/33112/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 15 lines to the logs
  • Reco comparison results: 59 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3190910
  • DQMHistoTests: Total failures: 2477
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3188411
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41932/35914

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

Pull request #41932 was updated. @smuzaffar, @Dr15Jones, @makortel, @nothingface0, @emanueleusai, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please check and sign again.

@ywkao
Copy link
Contributor Author

ywkao commented Jun 13, 2023

I think in addition the following places need to be amended accordingly

static TreeHelperBase* makeHelper(unsigned int iTypeIndex, TTree* iTree, std::string* iFullNameBufferPtr) {
switch (iTypeIndex) {
case kIntIndex:
return new IntTreeHelper(iTree, iFullNameBufferPtr);

m_treeReaders[kTProfileIndex].reset(new TreeObjectReader<TProfile>(MonitorElementData::Kind::TPROFILE, m_rescope));
m_treeReaders[kTProfile2DIndex].reset(
new TreeObjectReader<TProfile2D>(MonitorElementData::Kind::TPROFILE2D, m_rescope));
}

How about adding some tests to make sure all the I/O components work properly? (I don't remember if there are already any such tests that could be easily amended)

Okay, I will modify the lines and test it. Would PR #37665 be a good reference for the implementation of TH2Poly?

Also, I would appreciate any instructions from experts on how to test all the I/O components.

@makortel
Copy link
Contributor

Would PR #37665 be a good reference for the implementation of TH2Poly?

That PR indeeds looks like a good example.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41932/43917

@cmsbuild
Copy link
Contributor

Pull request #41932 was updated. @antoniovagnerini, @cmsbuild, @rseidita can you please check and sign again.

@ywkao
Copy link
Contributor Author

ywkao commented Feb 28, 2025

A draft PR #47478 is created to patch the TH2Poly::GetArray() functionality.

@antoniovagnerini
Copy link

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-03d614/44742/summary.html
COMMIT: 4fed83b
CMSSW: CMSSW_15_1_X_2025-02-27-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41932/44742/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 5 errors in the following unit tests:

---> test PrimaryVertex had ERRORS
---> test testPVPlotting had ERRORS
---> test test_PedeCampaign had ERRORS
and more ...

Comparison Summary

Summary:

  • You potentially added 1 lines to the logs
  • Reco comparison results: 7 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3920300
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3920275
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 214 log files, 184 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@ywkao
Copy link
Contributor Author

ywkao commented Mar 3, 2025

@antoniovagnerini, thank you for triggering the test.

I see that the test rejection is due to the failed comparison in two workflows (17034.0 and 25202.0). While there are comparison differences, I do not think they are caused by the latest commit, which only removes the commented-out lines. Some glitches may exist between the proposed commits and the workflows from the latest CMSSW.

Testing with CMSSW_15_0_X_2025-03-02-0000, I do not see any particular errors from the DQM unit test.

What can I do to make meaningful progress? For instance, should I test the affected workflows directly with the runTheMatrix.py commands? Or is there another debugging approach you would recommend?

@mmusich
Copy link
Contributor

mmusich commented Mar 3, 2025

@cmsbuild, please test

  • latest failures are unrelated

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2025

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-03d614/44763/summary.html
COMMIT: 4fed83b
CMSSW: CMSSW_15_1_X_2025-03-02-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41932/44763/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: 49
  • DQMHistoTests: Total histograms compared: 3920300
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3920277
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 214 log files, 184 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@ywkao
Copy link
Contributor Author

ywkao commented Mar 3, 2025

Hi @mmusich, thank you for the help. It looks the only pending check depends on #47478. Can you also help on that one?

@makortel
Copy link
Contributor

makortel commented Mar 3, 2025

I'm a bit confused about the plan forward with this PR and #47478 (that presently seems to contain this PR, and adds 3 other commits). Is the plan perhaps to merge this PR first, and then "refresh" #47478 to not contain the commits of this PR?

@ywkao
Copy link
Contributor Author

ywkao commented Mar 4, 2025

Is the plan perhaps to merge this PR first, and then "refresh" #47478 to not contain the commits of this PR?

@makortel, yes, that is the plan.

Some checks haven't completed yet
bot/47478/jenkinsWaiting for status to be reported — Waiting for authorized user to issue the test comma...

I notice there's still a pending check status for Jenkins on PR #47478, even though we've already run tests on that PR. Do you know how to properly clear this pending status, or is there something else we need to do to move forward?

@hqucms
Copy link
Contributor

hqucms commented Mar 4, 2025

Since #47478 includes everything of this PR plus more, and all tests have passed there, would it be much easier to simply consider this one superseded by #47478?

@cmsbuild cmsbuild merged commit 4fed83b into cms-sw:master Mar 10, 2025
14 of 15 checks passed
@github-project-automation github-project-automation bot moved this from Work in CMS to Done in ROOT prioritization Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.