Vin preshower #1195

VinInn · 2013-10-28T07:23:04Z

Seedup in Preshower and Ecal Raw2Digi code
see details inline

VinInn · 2013-10-28T07:24:18Z

messed up with Antoniio branch….
please remove them

VinInn · 2013-10-28T07:24:59Z

CalibCalorimetry/EcalLaserCorrection/src/EcalLaserDbService.cc

@@ -68,11 +69,16 @@ float EcalLaserDbService::getLaserCorrection (DetId const & xid, edm::Timestamp


  int iLM;
+  int xind;


"compute" hash index once

cmsbuild · 2013-10-28T07:27:45Z

A new Pull Request was created by @VinInn for CMSSW_7_0_X.

Vin preshower

It involves the following packages:

DataFormats/Provenance
RecoLocalCalo/EcalRecAlgos
CalibCalorimetry/EcalLaserCorrection
HLTrigger/Configuration
Geometry/CaloGeometry
DataFormats/EcalDetId
CondFormats/EcalObjects
Geometry/ForwardGeometry
RecoJets/JetProducers
RecoTracker/TkSeedGenerator
DataFormats/EcalRecHit

@perrotta, @smuzaffar, @apfeiffer1, @Dr15Jones, @demattia, @civanch, @fwyzard, @ianna, @mdhildreth, @Martin-Grunewald, @thspeer, @rcastello, @slava77, @ggovi, @ktf, @nclopezo can you please review it and eventually sign? Thanks.
@ghellwig, @TaiSakuma, @wmtan, @GiacomoSguazzoni, @rovere, @gpetruc, @ferencek, @cerati this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.

VinInn · 2013-10-28T07:30:22Z

CondFormats/EcalObjects/interface/EcalChannelStatusCode.h

@@ -18,16 +18,13 @@
 class EcalChannelStatusCode {

  public:
-    EcalChannelStatusCode();


in high pileup all these objects are constructed and destructed very often. better inline a constructor just hiding 16bits…

VinInn · 2013-10-28T07:38:54Z

I messed up my branch for this.

It includes changed already in previous pull request of mine (#1176)
and Antonio Tropiano's changes for PV seeding

These last one should be, if possible, removed from my pull request

vincenzo

On 28 Oct, 2013, at 8:27 AM, cmsbuild notifications@github.com wrote:

A new Pull Request was created by @VinInn for CMSSW_7_0_X.

Vin preshower

It involves the following packages:

DataFormats/Provenance
RecoLocalCalo/EcalRecAlgos
CalibCalorimetry/EcalLaserCorrection
HLTrigger/Configuration
Geometry/CaloGeometry
DataFormats/EcalDetId
CondFormats/EcalObjects
Geometry/ForwardGeometry
RecoJets/JetProducers
RecoTracker/TkSeedGenerator
DataFormats/EcalRecHit

@perrotta, @smuzaffar, @apfeiffer1, @Dr15Jones, @demattia, @civanch, @fwyzard, @ianna, @mdhildreth, @Martin-Grunewald, @thspeer, @rcastello, @slava77, @ggovi, @ktf, @nclopezo can you please review it and eventually sign? Thanks.
@ghellwig, @TaiSakuma, @wmtan, @GiacomoSguazzoni, @rovere, @gpetruc, @ferencek, @cerati this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.

VinInn · 2013-10-28T07:40:33Z

RecoLocalCalo/EcalRecAlgos/interface/ESRecHitSimAlgo.h

-  double w1_;
-  double w2_;
-  double MIPGeV_;
+  float w0_;


no reason to be double

…re to be done

cmsbuild · 2013-10-28T08:32:43Z

Pull request #1195 was updated. @smuzaffar, @apfeiffer1, @Dr15Jones, @demattia, @civanch, @ianna, @mdhildreth, @thspeer, @rcastello, @slava77, @ggovi, @ktf, @nclopezo can you please check and sign again.

VinInn · 2013-10-28T08:34:37Z

code in previous pull requests removed

Dr15Jones · 2013-10-28T13:39:03Z

+1

cmsbuild · 2013-10-31T18:00:06Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/1121/summary.html

apfeiffer1 · 2013-11-01T08:35:08Z

+1

thspeer · 2013-11-07T10:06:59Z

Working @thspeer

thspeer · 2013-11-07T14:21:02Z

-1
tested 1920cd3 in CMSSW_7_0_X_2013-11-07-0200
Small, but non-null, differences in ecal rechits, while no change is expected.

See for example in workflow 4.53 (RunPhoton2012B...) the energy:

thspeer · 2013-11-07T14:49:17Z

VinInn · 2013-11-07T17:33:01Z

could be that is the bad flag setting, not the energy value in itself

indeed
flag differ 5 6 5
flag differ 5 6 5
flag differ 5 6 5
flag differ 0 6 0
flag differ 0 6 0
flag differ 5 6 5
flag differ 0 6 0
flag differ 5 6 5
flag differ 0 6 0
flag differ 0 6 0
flag differ 0 6 0

it seems that the old code was flagging “6” quite often.
In my opinion now is more correct

the old and new energy are indentical…

this is a typical case
old adc for 6 3.000000 14.000000 5.000000 2.800000 2.800000
flag differ 0 6 0
ESd (ES z=1 plane 1 13:4 16) hashIndex = : 0.000118,0.000118 -25.090181,-25.090178 135.000000,135.000000
—

where the printout is this
double r23 = (adc[2] != 0) ? adc[1]/adc[2] : 99;
if (r23 > ratioCuts_->getR23High()) { status = 6;
std::cout << "old adc for 6 " << adc[0] << " " << adc[1] << " " << adc[2] << " " << r23 << " " << ratioCuts_->getR23High() << std::endl;
}

so r23 == ratioCuts_->getR23High()
but somehow in the old code (with double) managed to become larger….

btw all those intergers in double (or float) remains a bit messy

VinInn · 2013-11-08T08:51:23Z

to conclude: at least from my side:
the old code was considering ratios such as 14/5 28/10 etc as LARGER than 2.8 which is correct at least "face value".
This affects most probably only montecarlo (in data pedestals are not precise integers)

Because of this I ask the pull request to be accepted as is.

ktf · 2013-11-24T15:45:38Z

@thspeer since 70X is open again. Can you look again into it and change your vote?

rcastello · 2013-11-25T02:16:16Z

+1

argiro · 2013-11-25T08:08:28Z

On the Ecal side, the changes are Ok, the small differences do not seem cause of concern.

ktf · 2013-11-26T10:39:28Z

ping @thspeer

thspeer · 2013-11-26T13:25:45Z

+1
tested 1920cd3 in CMSSW_7_0_X_2013-11-07-0200
Small, but non-null, differences in ecal rechits, e.g. see plot above for energy, then on photons and conversions (very small)
Understood, and is ok for ecal.

cmsbuild · 2013-11-26T13:28:00Z

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

Reco updates -- Seedup in Preshower and Ecal Raw2Digi

VinInn reviewed Oct 28, 2013
View reviewed changes

VinInn added 10 commits October 28, 2013 09:16

compiles, removed multiple computation of indexing and new/delete, mo…

cff9ce8

…re to be done

move to float/constexpr

545f2bf

remove debug

cb8cdf8

move to vdt to ensure float algo

704ec40

moved inline constructor

082edb9

inlined

7b77e78

compute hash once

405adb2

ensure float

bf4fe60

inline, remove mutexes

68b4aa8

inlined

1920cd3

ktf added a commit that referenced this pull request Nov 26, 2013

Merge pull request #1195 from VinInn/VinPreshower

caeec85

Reco updates -- Seedup in Preshower and Ecal Raw2Digi

ktf merged commit caeec85 into cms-sw:CMSSW_7_0_X Nov 26, 2013

VinInn deleted the VinPreshower branch July 13, 2016 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vin preshower #1195

Vin preshower #1195

VinInn commented Oct 28, 2013

VinInn commented Oct 28, 2013

VinInn Oct 28, 2013

cmsbuild commented Oct 28, 2013

VinInn Oct 28, 2013

VinInn commented Oct 28, 2013

VinInn Oct 28, 2013

cmsbuild commented Oct 28, 2013

VinInn commented Oct 28, 2013

Dr15Jones commented Oct 28, 2013

cmsbuild commented Oct 31, 2013

apfeiffer1 commented Nov 1, 2013

thspeer commented Nov 7, 2013

thspeer commented Nov 7, 2013

thspeer commented Nov 7, 2013

VinInn commented Nov 7, 2013

VinInn commented Nov 8, 2013

ktf commented Nov 24, 2013

rcastello commented Nov 25, 2013

argiro commented Nov 25, 2013

ktf commented Nov 26, 2013

thspeer commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

		@@ -68,11 +69,16 @@ float EcalLaserDbService::getLaserCorrection (DetId const & xid, edm::Timestamp


		int iLM;
		int xind;

Vin preshower #1195

Vin preshower #1195

Conversation

VinInn commented Oct 28, 2013

VinInn commented Oct 28, 2013

VinInn Oct 28, 2013

Choose a reason for hiding this comment

cmsbuild commented Oct 28, 2013

VinInn Oct 28, 2013

Choose a reason for hiding this comment

VinInn commented Oct 28, 2013

VinInn Oct 28, 2013

Choose a reason for hiding this comment

cmsbuild commented Oct 28, 2013

VinInn commented Oct 28, 2013

Dr15Jones commented Oct 28, 2013

cmsbuild commented Oct 31, 2013

apfeiffer1 commented Nov 1, 2013

thspeer commented Nov 7, 2013

thspeer commented Nov 7, 2013

thspeer commented Nov 7, 2013

VinInn commented Nov 7, 2013

VinInn commented Nov 8, 2013

ktf commented Nov 24, 2013

rcastello commented Nov 25, 2013

argiro commented Nov 25, 2013

ktf commented Nov 26, 2013

thspeer commented Nov 26, 2013

cmsbuild commented Nov 26, 2013