cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard
verified publisher icon cms-sw

Implement TH2Poly in DQM Services for HGCal DQM

Open ywkao opened this issue 2 years ago • 52 comments

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] https://github.com/cms-DQM/dqmgui_prod/pull/14 [3] https://github.com/cms-DQM/dqmgui_prod/issues/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

ywkao avatar Jun 12 '23 15:06 ywkao

+code-checks

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

  • This PR adds an extra 44KB to repository

cmsbuild avatar Jun 12 '23 15:06 cmsbuild

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

cmsbuild avatar Jun 12 '23 15:06 cmsbuild

type hgcal

emanueleusai avatar Jun 13 '23 03:06 emanueleusai

please test

emanueleusai avatar Jun 13 '23 03:06 emanueleusai

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-03d614/33112/summary.html COMMIT: 5fa5e4eb27d3a585820be3f01d270f91939ee884 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 avatar Jun 13 '23 07:06 cmsbuild

+code-checks

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

  • This PR adds an extra 40KB to repository

cmsbuild avatar Jun 13 '23 16:06 cmsbuild

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

cmsbuild avatar Jun 13 '23 16:06 cmsbuild

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

https://github.com/cms-sw/cmssw/blob/e18c96dcf1c5d76ebcb075dc7b3446e92796ad6c/DQMServices/FwkIO/plugins/DQMRootOutputModule.cc#L250-L253

https://github.com/cms-sw/cmssw/blob/e18c96dcf1c5d76ebcb075dc7b3446e92796ad6c/DQMServices/FwkIO/plugins/DQMRootSource.cc#L465-L468

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.

ywkao avatar Jun 13 '23 17:06 ywkao

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

That PR indeeds looks like a good example.

makortel avatar Jun 13 '23 20:06 makortel

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

That PR indeeds looks like a good example.

Hi @makortel,

Following the example, I am implementing TH2Poly module-by-module in a standalone branch [1]. When modifying the files under DQMServices/Components, an error message related to TH2Poly::Add() appears:

DataFormats/Histograms/interface/MEtoEDMFormat.h:150:38: error: no matching function for call to 'TH2Poly::Add(const TH2Poly*)'

To fix the issue, one way I come up with is to add a condition for TH2Poly in the header file, as shown in [2].

+      } else if (MEtoEdmObject[j].object.IsA()->InheritsFrom("TH2Poly")) {
+        // ad-hoc addition because no matching function for call to 'TH2Poly::Add(const TH2Poly*)'
+        int nbins = MEtoEdmObject[j].object.GetNcells() - 9;
+        for(int ibin=1; ibin<nbins+1; ++ibin) {
+            double value1 = MEtoEdmObject[j].object.GetBinContent(ibin);
+            double value2 = newMEtoEDMObject[i].object.GetBinContent(ibin);
+            double total = value1 + value2;
+            MEtoEdmObject[j].object.SetBinContent(ibin, total);
+        }

However, another error message appears because the object is not a TObject as I expected:

error: 'struct {anonymous}::Dummy' has no member named 'IsA'

An entire compilation message can be found in the text file [3].

Do you have any suggestions on how to address the issue?

[1] https://github.com/ywkao/cmssw/commits/dev_th2poly [2] https://github.com/ywkao/cmssw/commit/002f7cb8fae76bdd8a61fbc15956e1129e5feb84#diff-6871d509700385eda0bc39736385861211ce0b8dfa759860e60ee3ad8dc005ae [3] https://ykao.web.cern.ch/ykao/raw_data_handling/pull_request_cmssw/log_compilation_MEtoEDMFormat_20230615.txt

ywkao avatar Jun 15 '23 14:06 ywkao

I suppose the use of Dummy comes from the test https://github.com/cms-sw/cmssw/blob/e0f88d0cd93d094405852b122ff61a2cbc9597a0/DataFormats/Histograms/test/metoedmformat_t.cc#L202-L224 In principle the mock class could be extended to provide IsA().

Written that, the ME type is known at compile time (T), so the best to have different behavior would be to leverage that. I see the mergeProduct() member function is already specialized for numbers and TString, so in principle you could follow that pattern. An alternative would be to use if constexpr.

What about the binning consistency checks? I'd imagine the TH2Poly would case would need to follow the established convention and merge the two objects only if their binning are consistent.

makortel avatar Jun 15 '23 18:06 makortel

Hello @makortel and DQM experts,

We encountered a technical issue: the TH2Poly does not have its copy constructor when specializing mergeProduct() for TH2Poly in DataFormats/Histograms/interface/MEtoEDMFormat.h. This results in an error message when calling the push_back() method:

error: call to implicitly-deleted copy constructor of 'MEtoEDM<TH2Poly>::MEtoEDMObject'

The issue has been reported on the root project accordingly [1].

After adding exceptions in the relevant parts [2][3], there still exist compilation errors [4]. It seems beyond what we can handle when trying to implement TH2Poly in DQMServices/Components following PR #37665.

Would you have any suggestions on this front?

[1] https://github.com/root-project/root/issues/13075 [2] throw cms::Exception("UnimplementedFeature") << "Not implemented for TH2Poly"; [3] https://github.com/ywkao/cmssw/commits/dev_th2poly_v2 [4] https://ykao.web.cern.ch/ykao/raw_data_handling/pull_request_cmssw/log_compilation_messages_20230622.txt

ywkao avatar Jun 22 '23 14:06 ywkao

@ywkao

As far as I can tell, all the compilation errors in

[4] https://ykao.web.cern.ch/ykao/raw_data_handling/pull_request_cmssw/log_compilation_messages_20230622.txt

stem from TH2Poly not having a copy constructor or copy assignment operator defined.

The first error comes from https://github.com/cms-sw/cmssw/blob/5f63e2de3276366becd0b442dad16e49f5cca1b9/DQMServices/Components/plugins/EDMtoMEConverter.cc#L480 On a quick look capturing metoedmobject as const reference (std::vector<typename MEtoEDM_T::MEtoEDMObject> const& metoedmobject = ...`) might work around that.

The second error originates from https://github.com/cms-sw/cmssw/blob/5f63e2de3276366becd0b442dad16e49f5cca1b9/DQMServices/Components/plugins/MEtoEDMConverter.cc#L429-L429 Even if the framework only requires movability of the data products, in the TH2Poly case the move constructor are not declared by the compiler because the copy constructor and assignment are declared.

I believe the best way would be for ROOT to make TH2Poly copyable or at least movable (tagging @pcanal for that in addition of the ROOT issue you opened). An alternative workaround would be to wrap TH2Poly into a class that has move (and possibly copy) operations defined.

makortel avatar Jun 22 '23 18:06 makortel

Milestone for this pull request has been moved to CMSSW_14_0_X.Please open a backport if it should also go in to CMSSW_13_3_X.

smuzaffar avatar Nov 06 '23 16:11 smuzaffar

please test

tjavaid avatar Nov 07 '23 14:11 tjavaid

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-03d614/35670/summary.html COMMIT: 14e856fb2cbd7915b8e34efd6e62007ba55dbdab CMSSW: CMSSW_14_0_X_2023-11-07-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41932/35670/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 24 lines to the logs
  • Reco comparison results: 140 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3363010
  • DQMHistoTests: Total failures: 2395
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3360593
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Nov 07 '23 18:11 cmsbuild

please test

tjavaid avatar Dec 16 '23 04:12 tjavaid

+1

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

Comparison Summary

Summary:

cmsbuild avatar Dec 16 '23 09:12 cmsbuild

please test

tjavaid avatar Dec 18 '23 09:12 tjavaid

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-03d614/36541/summary.html COMMIT: 14e856fb2cbd7915b8e34efd6e62007ba55dbdab CMSSW: CMSSW_14_0_X_2023-12-17-2300/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41932/36541/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 82 lines to the logs
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3429858
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3429833
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Dec 18 '23 12:12 cmsbuild

@ywkao

As far as I can tell, all the compilation errors in

[4] https://ykao.web.cern.ch/ykao/raw_data_handling/pull_request_cmssw/log_compilation_messages_20230622.txt

stem from TH2Poly not having a copy constructor or copy assignment operator defined.

The first error comes from

https://github.com/cms-sw/cmssw/blob/5f63e2de3276366becd0b442dad16e49f5cca1b9/DQMServices/Components/plugins/EDMtoMEConverter.cc#L480

On a quick look capturing metoedmobject as const reference (std::vector const& metoedmobject = ...`) might work around that.

The second error originates from

https://github.com/cms-sw/cmssw/blob/5f63e2de3276366becd0b442dad16e49f5cca1b9/DQMServices/Components/plugins/MEtoEDMConverter.cc#L429-L429

Even if the framework only requires movability of the data products, in the TH2Poly case the move constructor are not declared by the compiler because the copy constructor and assignment are declared.

I believe the best way would be for ROOT to make TH2Poly copyable or at least movable (tagging @pcanal for that in addition of the ROOT issue you opened). An alternative workaround would be to wrap TH2Poly into a class that has move (and possibly copy) operations defined.

Hi @ywkao , do you have a follow-up?

tjavaid avatar Dec 22 '23 12:12 tjavaid

Hi @ywkao , do you have a follow-up?

Hi @tjavaid, sorry for not updating it for a while (there was a period when we focused on the HGCAL beam test events). After a discussion with HGCAL experts, there is a preference for using the TH2Poly with a copy constructor patched in ROOT [1]. Wrapping TH2Poly into a class in the CMSSW seems feasible, but I do not know how to wrap it and am unsure if this is what we want for the long run.

@hqucms and @pfs can comment as well.

[1] https://github.com/root-project/root/issues/13075

ywkao avatar Dec 22 '23 13:12 ywkao

Hi, sorry for late feedback. @tjavaid do you have a recommendation on how the wrapper would be? Would it suffice to have a class derived from TH2Poly which implements the copy CTOR? In some sense however i have the feeling that this is something that ROOT developers should have implemented so i'll push on the thread included by @ywkao as well

pfs avatar Jan 15 '24 08:01 pfs

type root

makortel avatar Jan 19 '24 17:01 makortel

I'd suggest to look into composition rather than inheritance, and limiting its use to only where it is needed (hopefully only for interfacing with the framework). In principle storing TH2Poly as std::unique_ptr<TH2Poly> might work.

makortel avatar Jan 19 '24 17:01 makortel

Milestone for this pull request has been moved to CMSSW_14_1_X. Please open a backport if it should also go in to CMSSW_14_0_X.

cmsbuild avatar Feb 06 '24 10:02 cmsbuild

out of curiosity (since this can be potentially be used by other sub-systems), what's the plan to integrate this? looks like https://github.com/root-project/root/issues/13075 was closed via https://github.com/root-project/root/pull/14732. Do we already have a root version with that feature included employed for cmssw?

mmusich avatar Apr 02 '24 11:04 mmusich

looks like root-project/root#13075 was closed via root-project/root#14732. Do we already have a root version with that feature included employed for cmssw?

That ROOT PR is included in our ROOT master (ROOT6) IBs. In order to have the feature "generally available" we'd need a backport of that PR into ROOT 6.30 branch.

makortel avatar Apr 02 '24 13:04 makortel

In order to have the feature "generally available" we'd need a backport of that PR into ROOT 6.30 branch.

will someone from core push for that in the root project? or is left for the users?

mmusich avatar Apr 02 '24 13:04 mmusich

In order to have the feature "generally available" we'd need a backport of that PR into ROOT 6.30 branch.

will someone from core push for that in the root project? or is left for the users?

Let's see what they reply to https://github.com/root-project/root/pull/14732#issuecomment-2032098659 .

makortel avatar Apr 02 '24 13:04 makortel