Extended Multifit functionality #17205

bendavid · 2017-01-18T23:26:40Z

Extends ecal multifit functionality with possibility for

Dynamic pedestals
Inflated pedestal uncertainty at level of python configuration
Better handling of noise covariance matrix for gain switch rechits
Ability to remove single sample from the fit to mitigate slew rate limitation

More information and background in
https://indico.cern.ch/event/600314/contributions/2441465/attachments/1398120/2132187/multifit-Jan18-2017.pdf

All of the new options are disabled by default, and therefore this pull request is expected to give 1:1 identical results for all standard workflows (including HLT). There may be a small cpu and/or memory increase as the size on the stack of some of the matrix types have been increased to accommodate the new functionality.

This should not necessarily be merged yet, pending further tests and understanding of plans for 2017 data/mc, but want to at least run through the automated tests and verify 1:1 identity with present settings.

@emanueledimarco @amassiro @lgray @VinInn

…lew limit handling for multifit

…limited samples

…lt anyway), disable dynamic pedestals in case of gain switch, and enable dynamic pedestals at beginning of NNLS minimiazation by default

…ults to strictly reproduce existing behaviour

cmsbuild · 2017-01-18T23:27:14Z

A new Pull Request was created by @bendavid (Josh Bendavid) for CMSSW_9_0_X.

It involves the following packages:

RecoLocalCalo/EcalRecAlgos
RecoLocalCalo/EcalRecProducers

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@argiro this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

slava77 · 2017-01-19T00:26:05Z

@cmsbuild please test

cmsbuild · 2017-01-19T00:26:18Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17322/console Started: 2017/01/19 01:27

cmsbuild · 2017-01-19T02:04:32Z

+1
Tested at: f4837b2
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17205/17322/summary.html

cmsbuild · 2017-01-19T02:04:35Z

Comparison job queued.

cmsbuild · 2017-01-19T03:15:08Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17205/17322/summary.html

slava77 · 2017-01-19T15:35:11Z

RecoLocalCalo/EcalRecAlgos/interface/EcalUncalibRecHitMultiFitAlgo.h

  void disableErrorCalculation() { _computeErrors = false; }
  void setDoPrefit(bool b) { _doPrefit = b; }
  void setPrefitMaxChiSq(double x) { _prefitMaxChiSq = x; }
+  void setDynamicPedestals(bool b=true) { _dynamicPedestals = b; }
+  void setMitigateBadSamples(bool b=true) { _mitigateBadSamples = b; }
+  void setSelectiveBadSampleCriteria(bool b=true) { _selectiveBadSampleCriteria = b; }


please remove default values or introduce set and unset methods if no-argument calls are desired.

Ok will do that.

slava77 · 2017-01-19T15:36:00Z

RecoLocalCalo/EcalRecAlgos/interface/EigenMatrixTypes.h

@@ -1,16 +1,30 @@
+#ifndef EigenMatrixTypes_h
+#define EigenMatrixTypes_h


please use a more specific include guard RecoLocalCalo_EcalRecAlgos_EigenMatrixTypes_h

or move them to DataFormats/Math
ok, no. too specifics (at least the naming)

Yes ok I'll leave them here and just change the ifndef as suggested.
The actual numbers which are here are pretty detector and algorithm specific, so I think it makes sense to keep them here anyways.

slava77 · 2017-01-19T16:00:29Z

RecoLocalCalo/EcalRecAlgos/src/EigenMatrixTypes.cc

@@ -0,0 +1,2 @@
+#include "RecoLocalCalo/EcalRecAlgos/interface/EigenMatrixTypes.h"


is this file actually needed? EigenMatrixTypes.h contains only typedefs and constexpr.

Yes, agreed that it is not needed, will remove it.

slava77 · 2017-01-19T16:03:37Z

RecoLocalCalo/EcalRecAlgos/src/PulseChiSqSNNLS.cc

@@ -79,51 +79,82 @@ PulseChiSqSNNLS::PulseChiSqSNNLS() :
  _maxiterwarnings(true)
 {

-  Eigen::initParallel();


https://eigen.tuxfamily.org/dox/TopicMultiThreading.html

In the case your own application is multithreaded, and multiple threads make calls to Eigen, then you have to initialize Eigen by calling the following routine before creating the threads

Is removal justified?

Yes, this can be removed as per the discussion at https://hypernews.cern.ch/HyperNews/CMS/get/swDevelopment/3290/1/1/1/1/1/1/1/1.html and the change here cms-externals/eigen#1

@Dr15Jones can comment further

It appears @bendavid is correct.

cmsbuild · 2017-01-19T19:50:20Z

Pull request #17205 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

bendavid · 2017-01-19T19:51:46Z

Minor cleanup added according to comments.

slava77 · 2017-01-19T21:44:36Z

@cmsbuild please test

cmsbuild · 2017-01-19T21:44:51Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17335/console Started: 2017/01/19 22:46

cmsbuild · 2017-01-19T22:34:35Z

+1
Tested at: eb0cf93
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17205/17335/summary.html

cmsbuild · 2017-01-19T22:34:38Z

Comparison job queued.

cmsbuild · 2017-01-19T23:07:13Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17205/17335/summary.html

slava77 · 2017-01-22T19:39:24Z

RecoLocalCalo/EcalRecAlgos/src/PulseChiSqSNNLS.cc


  const unsigned int nsample = SampleVector::RowsAtCompileTime;
  const unsigned int npulse = _bxs.rows();

-  const double pederr2 = pederr*pederr;
-  _invcov = pederr2*samplecor; //
+  _invcov = samplecov; //


it looks like this part and the way samplecov is now computed upstream lead to some differences at numerical precision level observed in DQM comparisons.

pederr2 was (pederr*gainratio)*(pederr*gainratio), while now it is (pederr* pederr* gainratio* gainratio)

the other part of the logic that appears different is abs(bx)>=100 added in a few places, but IIUC, this never happens with the current defaults.
@bendavid
please confirm

slava77 · 2017-01-22T19:52:31Z

looking at jenkins DQM plots: there are changes starting from ECAL energies with differences at the level of float math precision

I ran 136.761 with 200 events locally and see
e.g.

en->Scan("EcalRecHitsSorted_ecalRecHit_EcalRecHitsEE_reRECO.obj.obj.energy():\
eo.EcalRecHitsSorted_ecalRecHit_EcalRecHitsEE_reRECO.obj.obj.energy():\
EcalRecHitsSorted_ecalRecHit_EcalRecHitsEE_reRECO.obj.obj.energy()-eo.EcalRecHitsSorted_ecalRecHit_EcalRecHitsEE_reRECO.obj.obj.energy()",\
 "EcalRecHitsSorted_ecalRecHit_EcalRecHitsEE_reRECO.obj.obj.energy()!=eo.EcalRecHitsSorted_ecalRecHit_EcalRecHitsEE_reRECO.obj.obj.energy()")
***********************************************************
*    Row   * Instance * EcalRecHi * eo.EcalRe * EcalRecHi *
***********************************************************
*       10 *      477 * 0.0106320 * 0.0106320 * 9.313e-10 *
*       11 *      464 * 2.8491241 * 2.8491237 * 4.768e-07 *
*       16 *      940 * 0.0003039 * 0.0003039 * -2.91e-11 *
*       17 *      353 * 0.0058631 * 0.0058631 * 4.656e-10 *
*       20 *      392 * 0.3206158 * 0.3206158 * -2.98e-08 *
*       21 *      537 * 6.178e-05 * 6.178e-05 * 7.275e-12 *
*       22 *       28 * 0.0018613 * 0.0018613 * 2.328e-10 *

there are 43 hits with different energies in EE and 123 in EB.
These differences propagate downstream: ~12 pfCandidates have different momentum.

If this is all just from pederr2 (noisecov) computed with a semantically different formula #17205 (comment)
I can sign off on this.

If there is some other logic change that might create larger differences for some rare hits (e.g. the ones with gainswitch), then it would be better to understand what the effect on those would be.

bendavid · 2017-01-22T20:08:51Z

Agreed that changes at the level of numerical precision are possible/expected. Also confirmed that abs(bx)>=100 never occurs presently (this only happens if dynamic pedestals are enabled)

The effect on gain switch rechits should be visible in an eg. 1tev electron/photon gun or very high mass Zprime->ee sample, but here also differences for standard workflows should be only at the level of numerical precision. (To be checked whether there are already dqm plots which isolate the gain switch rechits, otherwise we probably need to explicitly check by hand, since there are some technical changes here which only affect the gain switch case, even if they should not change anything beyond numerical precision with present settings as said.)

slava77 · 2017-01-23T17:48:53Z

+1

for #17205 f4837b2

code changes are in line with the description
jenkins tests pass and comparisons with the baseline show differences in ECAL energies at the level of numerical precision (expected from the changes done) which are also visible in downstream objects
local test with 1TeV ele gun shows only differences at numerical precision level

cmsbuild · 2017-01-23T17:49:09Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

davidlange6 · 2017-01-23T19:38:15Z

+1

bendavid added 8 commits December 19, 2016 18:43

first implementation of dynamic pedestals and proper saturation and s…

d8af01f

…lew limit handling for multifit

fix configuration switches and refine treatment of saturated or slew-…

d0fd17c

…limited samples

bug fix and optimization

748a26f

remove support for saturation in multifit (multifit bypassed by defau…

1395df2

…lt anyway), disable dynamic pedestals in case of gain switch, and enable dynamic pedestals at beginning of NNLS minimiazation by default

add configurable inflation of pedestal uncertainties

38d381b

disable dynamic pedestals by default

146e900

remove commented debug printouts

8ebfd6b

make additional multifit functionality more configurable and set defa…

f4837b2

…ults to strictly reproduce existing behaviour

cmsbuild added this to the Next CMSSW_9_0_X milestone Jan 18, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 18, 2017

cmsbuild added tests-started and removed tests-pending labels Jan 19, 2017

cmsbuild added tests-approved and removed tests-started labels Jan 19, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jan 19, 2017

slava77 reviewed Jan 19, 2017

View reviewed changes

cmsbuild added comparison-pending tests-pending labels Jan 19, 2017

cmsbuild added tests-started and removed tests-pending labels Jan 19, 2017

cmsbuild added tests-approved and removed tests-started labels Jan 19, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jan 19, 2017

slava77 reviewed Jan 22, 2017

View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jan 23, 2017

cmsbuild added orp-approved and removed orp-pending labels Jan 23, 2017

cmsbuild merged commit 06c3837 into cms-sw:CMSSW_9_0_X Jan 23, 2017

This was referenced Jan 24, 2017

Use of Max-Sample algorithm for gain-switch cases in the barrel #17259

Merged

Optional use of Max-Sample algorithm for gain-switch cases #17279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended Multifit functionality #17205

Extended Multifit functionality #17205

bendavid commented Jan 18, 2017

cmsbuild commented Jan 18, 2017

slava77 commented Jan 19, 2017

cmsbuild commented Jan 19, 2017 •

edited

Loading

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

slava77 Jan 19, 2017

bendavid Jan 19, 2017

slava77 Jan 19, 2017

VinInn Jan 19, 2017 •

edited

Loading

bendavid Jan 19, 2017

slava77 Jan 19, 2017

bendavid Jan 19, 2017

slava77 Jan 19, 2017

bendavid Jan 19, 2017

Dr15Jones Jan 19, 2017

cmsbuild commented Jan 19, 2017

bendavid commented Jan 19, 2017

slava77 commented Jan 19, 2017

cmsbuild commented Jan 19, 2017 •

edited

Loading

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

slava77 Jan 22, 2017

slava77 commented Jan 22, 2017

bendavid commented Jan 22, 2017

slava77 commented Jan 23, 2017 •

edited

Loading

cmsbuild commented Jan 23, 2017

davidlange6 commented Jan 23, 2017

		@@ -1,16 +1,30 @@
		#ifndef EigenMatrixTypes_h
		#define EigenMatrixTypes_h

		@@ -0,0 +1,2 @@
		#include "RecoLocalCalo/EcalRecAlgos/interface/EigenMatrixTypes.h"

Extended Multifit functionality #17205

Extended Multifit functionality #17205

Conversation

bendavid commented Jan 18, 2017

cmsbuild commented Jan 18, 2017

slava77 commented Jan 19, 2017

cmsbuild commented Jan 19, 2017 • edited Loading

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jan 19, 2017

bendavid commented Jan 19, 2017

slava77 commented Jan 19, 2017

cmsbuild commented Jan 19, 2017 • edited Loading

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

cmsbuild commented Jan 19, 2017

Choose a reason for hiding this comment

slava77 commented Jan 22, 2017

bendavid commented Jan 22, 2017

slava77 commented Jan 23, 2017 • edited Loading

cmsbuild commented Jan 23, 2017

davidlange6 commented Jan 23, 2017

cmsbuild commented Jan 19, 2017 •

edited

Loading

VinInn Jan 19, 2017 •

edited

Loading

cmsbuild commented Jan 19, 2017 •

edited

Loading

slava77 commented Jan 23, 2017 •

edited

Loading