Fixing Stat Overflows consideration on DQM modules #32364

jfernan2 · 2020-12-02T09:47:07Z

PR description:

This PR tries to fix the problem mentioned in #32343 (comment)

The proposed solution goes directly to DQM core setStatOverflows method instead of using teh ROOT method SetStatOverflow since DQM Monitor Elements inherit from there and this prevents modifying many DQM modules around.

I let coding experts to decide if this solution is preferred to directly modify
DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc
DQM/SiStripMonitorTrack/src/SiStripMonitorTrack.cc
DQM/SiStripMonitorDigi/src/SiStripMonitorDigi.cc
DQMOffline/EGamma/src/ElectronDqmAnalyzerBase.cc
as suggested on #3234 instead.

I can switch to that strategy if preferred.

I have also noticed that there are many Alignment classes suffering from this too:
https://github.com/cms-sw/cmssw/search?p=1&q=StatOverflows

PR validation:

Checked compilation and some wf running

jfernan2 · 2020-12-02T09:47:18Z

please test

cmsbuild · 2020-12-02T09:54:32Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32364/20209

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)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32364/20209/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32364/20209/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

mmusich · 2020-12-02T10:38:48Z

@jfernan2

I have also noticed that there are many Alignment classes suffering from this too:
https://github.com/cms-sw/cmssw/search?p=1&q=StatOverflows

none of those (Alignment classes and macros) is run in central workflows, so I think they cannot do any harm anywhere else.
About:

DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc
DQM/SiStripMonitorTrack/src/SiStripMonitorTrack.cc
DQM/SiStripMonitorDigi/src/SiStripMonitorDigi.cc

I think there is an ongoing campaign to remove every direct access to the underlying bare ROOT from the ME within the tracker DQM group (see e.g: this talk )
Wouldn't solution at #32343 going into the opposite direction?
(I am genuinely asking, as I am not expert)
@sroychow @arossi83

jfernan2 · 2020-12-02T10:45:01Z

Thanks @mmusich I am open to implement the other solution, I do not know which one is best

mmusich · 2020-12-02T10:51:51Z

I do not know which one is best.

IMHO the one you propose here is the better one, but I am definitely not an expert.

cmsbuild · 2020-12-02T10:54:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32364/20210

This PR adds an extra 20KB to repository

cmsbuild · 2020-12-02T10:55:46Z

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

It involves the following packages:

DQMOffline/EGamma
DQMServices/Core

@andrius-k, @kmaeshima, @ErnestaP, @cmsbuild, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks.
@barvic, @rovere, @lecriste, @rociovilar this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

jfernan2 · 2020-12-02T10:57:32Z

please test
Let's see if this solution works without changes

cmsbuild · 2020-12-02T10:57:58Z

The tests are being triggered in jenkins.

CMSSW_11_3_X_2020-12-01-2300/slc7_amd64_gcc900: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/11253/console Started: 2020/12/02 12:00

cmsbuild · 2020-12-02T12:23:11Z

+1
Tested at: b70c590
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13b23a/11253/summary.html
CMSSW: CMSSW_11_3_X_2020-12-01-2300
SCRAM_ARCH: slc7_amd64_gcc900

missirol · 2020-12-15T11:14:00Z

the approach used in #32343 seems to be in direct contradiction with earlier recommendations from the central DQM group.

One clarification from my side (for what it's worth): I was not aware of those DQM recommendations (@jfernan2 , could you please point to them?). What was done in #32343 was the only way, at that time, to avoid using ROOT's StatOverflows (I think using SetStatOverflows is the better way, since it doesn't cause changes to the outputs of unrelated modules). Of course, if a call to SetStatOverflows becomes available via MonitorElement (as done in this PR), there will be a followup to #32343 to use that instead of doing it via getTH1().

jfernan2 · 2020-12-15T11:39:56Z

@missirol in principle DQM methods are working on internal "ROOT" objects:

cmssw/DQMServices/Core/interface/MonitorElement.h

Lines 370 to 430 in d2c2ace

    
           // const and data-independent -- safe 
        
           virtual int getNbinsX() const; 
        
           virtual int getNbinsY() const; 
        
           virtual int getNbinsZ() const; 
        
           virtual int getBin(int binx, int biny) const; 
        
           virtual std::string getAxisTitle(int axis = 1) const; 
        
           virtual std::string getTitle() const; 
        
           // const but data-dependent -- semantically unsafe in RECO 
        
           virtual double getMean(int axis = 1) const; 
        
           virtual double getMeanError(int axis = 1) const; 
        
           virtual double getRMS(int axis = 1) const; 
        
           virtual double getRMSError(int axis = 1) const; 
        
           virtual double getBinContent(int binx) const; 
        
           virtual double getBinContent(int binx, int biny) const; 
        
           virtual double getBinContent(int binx, int biny, int binz) const; 
        
           virtual double getBinError(int binx) const; 
        
           virtual double getBinError(int binx, int biny) const; 
        
           virtual double getBinError(int binx, int biny, int binz) const; 
        
           virtual double getEntries() const; 
        
           virtual double getBinEntries(int bin) const; 
        
           virtual double getBinEntries(int binx, int biny) const; 
        
           virtual double integral() const; 
        
           virtual int64_t getIntValue() const; 
        
           virtual double getFloatValue() const; 
        
           virtual const std::string &getStringValue() const; 
        
           // non-const -- thread safety and semantical issues 
        
           virtual void setBinContent(int binx, double content); 
        
           virtual void setBinContent(int binx, int biny, double content); 
        
           virtual void setBinContent(int binx, int biny, int binz, double content); 
        
           virtual void setBinError(int binx, double error); 
        
           virtual void setBinError(int binx, int biny, double error); 
        
           virtual void setBinError(int binx, int biny, int binz, double error); 
        
           virtual void setBinEntries(int bin, double nentries); 
        
           virtual void setEntries(double nentries); 
        
           virtual void divide(const MonitorElement *, const MonitorElement *, double, double, const char *); 
        
           virtual void setBinLabel(int bin, const std::string &label, int axis = 1); 
        
           virtual void setAxisRange(double xmin, double xmax, int axis = 1); 
        
           virtual void setAxisTitle(const std::string &title, int axis = 1); 
        
           virtual void setAxisTimeDisplay(int value, int axis = 1); 
        
           virtual void setAxisTimeFormat(const char *format = "", int axis = 1); 
        
           virtual void setTitle(const std::string &title); 
        
           // additional operations mainly for booking 
        
           virtual void setXTitle(std::string const &title); 
        
           virtual void setYTitle(std::string const &title); 
        
           virtual void enableSumw2(); 
        
           virtual void disableAlphanumeric(); 
        
           virtual void setOption(const char *option); 
        
           virtual double getAxisMin(int axis = 1) const; 
        
           virtual double getAxisMax(int axis = 1) const; 
        
           // We should avoid extending histograms in general, and if the behaviour 
        
           // is actually needed, provide a more specific interface rather than 
        
           // relying on the ROOT behaviour. 
        
           DQM_DEPRECATED 
        
           virtual void setCanExtend(unsigned int value); 
        
           // We should decide if we support this (or make it default) 
        
           DQM_DEPRECATED 
        
           virtual void setStatOverflows(unsigned int value);

so, to avoid colliding modifications which are difficult to track, direct ROOT modifications are not desired

jfernan2 · 2021-01-15T14:06:54Z

ping @sroychow @arossi83

mmusich · 2021-01-18T15:38:54Z

@cmsbuild please test

mmusich · 2021-01-18T15:39:53Z

@jfernan2,
Alessandro (@arossi83) is trying to reproduce locally but doens't seem to see the same things as the bot. I am retriggering the tests in order to get a fresh comparison.

cmsbuild · 2021-01-18T18:16:54Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13b23a/12328/summary.html
COMMIT: d2c2ace
CMSSW: CMSSW_11_3_X_2021-01-17-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716961
DQMHistoTests: Total failures: 4271
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2712668
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 156 log files, 37 edm output root files, 37 DQM output files

arossi83 · 2021-01-22T11:05:08Z

@jfernan2

We investigate a little bit the difference we see, in particular on PixelPhase1 plots.
The differences are due to the fact that now the SetStatsOverflows applies only to the specific ME and not on the Global behavior of the ROOT object as before. Due to this PixelPhase1 plots are by default with StatsOverflows disabled.

I introduce a quick&dirty patch to verify this by manually enabling StatsOverflows for PixelPhase1 plots. In attachment you can find a plot with overlayed this PR, the baseline and the PR+Patch. You can see that by re-enabling the StatsOverflows no changes arise for Phase1 plots.

In light of that the hold can be removed from this PR so that it can be merged. We will fix the TrackerDQM part with a separate PR.

jfernan2 · 2021-01-22T11:07:10Z

Thank you very much @arossi83 !!

jfernan2 · 2021-01-22T11:07:16Z

unhold

jfernan2 · 2021-01-22T11:10:07Z

@amassiro would you be so kind to perform the followup to #32343 you mentioned below? Thanks

One clarification from my side (for what it's worth): I was not aware of those DQM recommendations (@jfernan2 , could you please point to them?). What was done in #32343 was the only way, at that time, to avoid using ROOT's StatOverflows (I think using SetStatOverflows is the better way, since it doesn't cause changes to the outputs of unrelated modules). Of course, if a call to SetStatOverflows becomes available via MonitorElement (as done in this PR), there will be a followup to #32343 to use that instead of doing it via getTH1().

missirol · 2021-01-22T11:11:02Z

@amassiro would you be so kind to perform the followup to #32343 you mentioned below? Thanks

(i guess the message was for me.. :))

Yes, will do, as soon as this is merged.

jfernan2 · 2021-01-22T11:12:43Z

Oh yes, sorry, github misled me. Thank you @missirol !

jfernan2 · 2021-01-22T11:12:47Z

+1

cmsbuild · 2021-01-22T11:13:07Z

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-01-22T14:01:26Z

+1

jfernan2 added 2 commits December 2, 2020 10:38

Modifying Core class to follow PR32343 finding

aff6b4f

Move to DQM core class method

728a736

cmsbuild added this to the CMSSW_11_3_X milestone Dec 2, 2020

cmsbuild added code-checks-pending comparison-pending dqm-pending orp-pending pending-signatures tests-pending labels Dec 2, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 2, 2020

Code checks

b70c590

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 2, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 2, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 2, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 2, 2020

cmsbuild added tests-started and removed tests-approved labels Jan 18, 2021

cmsbuild added tests-approved and removed tests-started labels Jan 18, 2021

cmsbuild removed the hold label Jan 22, 2021

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Jan 22, 2021

cmsbuild added orp-approved and removed orp-pending labels Jan 22, 2021

cmsbuild merged commit 528ad8a into cms-sw:master Jan 22, 2021

jfernan2 mentioned this pull request Jan 22, 2021

Add setStatOverflows to get the mean value including the overflow #32720

Merged

missirol mentioned this pull request Jan 24, 2021

FastTimerService: removed explicit calls to MonitorElement::getTH1 #32726

Merged

arossi83 mentioned this pull request Jan 28, 2021

StatOverflows on PixelPhase1 DQM #32761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Stat Overflows consideration on DQM modules #32364

Fixing Stat Overflows consideration on DQM modules #32364

jfernan2 commented Dec 2, 2020 •

edited

jfernan2 commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

mmusich commented Dec 2, 2020

jfernan2 commented Dec 2, 2020 •

edited

mmusich commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

jfernan2 commented Dec 2, 2020

cmsbuild commented Dec 2, 2020 •

edited

cmsbuild commented Dec 2, 2020

missirol commented Dec 15, 2020

jfernan2 commented Dec 15, 2020

jfernan2 commented Jan 15, 2021

mmusich commented Jan 18, 2021

mmusich commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

arossi83 commented Jan 22, 2021 •

edited

jfernan2 commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

missirol commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

cmsbuild commented Jan 22, 2021

qliphy commented Jan 22, 2021

Fixing Stat Overflows consideration on DQM modules #32364

Fixing Stat Overflows consideration on DQM modules #32364

Conversation

jfernan2 commented Dec 2, 2020 • edited

PR description:

PR validation:

jfernan2 commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

mmusich commented Dec 2, 2020

jfernan2 commented Dec 2, 2020 • edited

mmusich commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

cmsbuild commented Dec 2, 2020

jfernan2 commented Dec 2, 2020

cmsbuild commented Dec 2, 2020 • edited

cmsbuild commented Dec 2, 2020

missirol commented Dec 15, 2020

jfernan2 commented Dec 15, 2020

jfernan2 commented Jan 15, 2021

mmusich commented Jan 18, 2021

mmusich commented Jan 18, 2021

cmsbuild commented Jan 18, 2021

Comparison Summary

arossi83 commented Jan 22, 2021 • edited

jfernan2 commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

missirol commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

jfernan2 commented Jan 22, 2021

cmsbuild commented Jan 22, 2021

qliphy commented Jan 22, 2021

jfernan2 commented Dec 2, 2020 •

edited

jfernan2 commented Dec 2, 2020 •

edited

cmsbuild commented Dec 2, 2020 •

edited

arossi83 commented Jan 22, 2021 •

edited