Skip to content
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

HGCal Reworked rechit scheme #13835

Merged
merged 5 commits into from Apr 6, 2016
Merged

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Mar 24, 2016

HGC Rechits are reworked such that uncalibrated Rechits are in MIPs.

This is preparation towards having an EM calibrated RecHit.

Also, the hexagon geometry is made default in unit tests from now on.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_8_1_X.

It involves the following packages:

RecoLocalCalo/HGCalRecAlgos
RecoLocalCalo/HGCalRecProducers
RecoParticleFlow/PFClusterProducer
SimCalorimetry/HGCalSimProducers

@civanch, @cvuosalo, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@vandreev11, @sethzenz, @mmarionncern, @kpedro88, @argiro, @cseez, @pfs, @bachtis this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@lgray
Copy link
Contributor Author

lgray commented Mar 24, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12061/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Mar 28, 2016

+1

HGCalDetId hid(dataFrame.id());
thickness = ddd_->waferTypeL(hid.wafer());
}
amplitude_ = amplitude_/fCPerMIP_[thickness-1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if fCPerMIP_[thickness-1] is zero before dividing by it.

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 will check this in the constructor so it is guaranteed non-zero here.

@cvuosalo
Copy link
Contributor

The static analyzer complains about SimCalorimetry/HGCalSimProducers/src/HGCHEbackDigitizer.cc. It would be nice to get the issue fixed at some point. Lines 59-64 could be changed as follows.

59    float totalMIPs(totalIniMIPs);

Value stored to 'totalMIPs' during its initialization is never read
60        const float xtalk = (nTotalPE_-xTalk_*((float)nPixel))/(nTotalPE_-((float)nPixel));
61        if( nTotalPE_ != nPixel && xtalk > 0. )
62          totalMIPs = (nTotalPE_/nPEperMIP_)*vdt::fast_logf(xtalk);
63        else
64          totalMIPs = 0.f;

to

float totalMIPs(0.f), 
const float xtalk = 0.f,  peDiff = nTotalPE_ - (float) nPixel;
if (peDiff != 0.) {
  xtalk = (nTotalPE_-xTalk_*((float)nPixel)) / peDiff;
  if( xtalk > 0. && nPEperMIP_ != 0.)
   totalMIPs = (nTotalPE_/nPEperMIP_)*vdt::fast_logf(xtalk);
}

It could be the line totalMIPs = 0.f; is incorrect, so the initialization float totalMIPs(totalIniMIPs); would actually be correct.

@lgray
Copy link
Contributor Author

lgray commented Apr 4, 2016

@cvuosalo Thanks for the review. I am taking care of it now.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2016

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

@lgray
Copy link
Contributor Author

lgray commented Apr 4, 2016

@cmsbuild please test

I have addressed the code review.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12163/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2016

@cvuosalo
Copy link
Contributor

cvuosalo commented Apr 4, 2016

+1

For #13835 4f5269c

Revising HGCal rec hits. There should be no change in standard (non-HGCal) workflows.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-04-04-1100 show no significant differences, as expected.

@civanch
Copy link
Contributor

civanch commented Apr 6, 2016

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 6, 2016

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit ccf2225 into cms-sw:CMSSW_8_1_X Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants