add vertex covariance for V0Producer 76x #12142

fojensen · 2015-10-28T15:00:35Z

same as
75x
#12133
80x
#12134
bug fix in V0Producer

cmsbuild · 2015-10-28T15:01:15Z

A new Pull Request was created by @fojensen (Frank Jensen) for CMSSW_7_6_X.

add vertex covariance for V0Producer 76x

It involves the following packages:

RecoVertex/V0Producer

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @cerati, @dgulhan 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

slava77 · 2015-10-28T15:02:14Z

@cmsbuild please test

VinInn · 2015-10-28T15:05:31Z

RecoVertex/V0Producer/src/V0Fitter.cc

-         ipsigXY = std::abs(tmpTrack->dxy(*theBeamSpot)/tmpTrack->dxyError()); 
-      }
+      double ipsigXY = std::abs(tmpTrack->dxy(*theBeamSpot)/tmpTrack->dxyError());
+      if (useVertex_) ipsigXY = std::abs(tmpTrack->dxy(referencePos)/tmpTrack->dxyError());


why computing twice ipsingXY in case of useVertex_?
was not the previous code more efficient?

On 10/28/15 10:05 AM, Vincenzo Innocente wrote:

In RecoVertex/V0Producer/src/V0Fitter.cc
#12142 (comment):

@@ -116,12 +117,8 @@ void V0Fitter::fitAll(const edm::Event& iEvent, const edm::EventSetup& iSetup,
// fill vectors of TransientTracks and TrackRefs after applying preselection cuts
for (reco::TrackCollection::const_iterator iTk = theTrackCollection->begin(); iTk != theTrackCollection->end(); ++iTk) {
const reco::Track* tmpTrack = &(*iTk);

double ipsigXY;

if (useVertex_) {

ipsigXY = std::abs(tmpTrack->dxy(referencePos)/tmpTrack->dxyError());

} else {

ipsigXY = std::abs(tmpTrack->dxy(*theBeamSpot)/tmpTrack->dxyError());

}

double ipsigXY = std::abs(tmpTrack->dxy(*theBeamSpot)/tmpTrack->dxyError());

if (useVertex_) ipsigXY = std::abs(tmpTrack->dxy(referencePos)/tmpTrack->dxyError());

why computing twice ipsingXY in case of useVertex_?
was not the previous code more efficient?

if/else is probably going to be more CPU efficient
(unclear though how the compiler puts the conditional computations in
optimization)

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/12142/files#r43267434.

cmsbuild · 2015-10-28T15:06:09Z

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

VinInn · 2015-10-28T15:06:38Z

RecoVertex/V0Producer/src/V0Fitter.cc

@@ -206,7 +203,8 @@ void V0Fitter::fitAll(const edm::Event& iEvent, const edm::EventSetup& iSetup,
      GlobalPoint vtxPos(theVtx.x(), theVtx.y(), theVtx.z());

      // 2D decay significance
-      const SMatrixSym3D totalCov = theBeamSpot->rotatedCovariance3D() + theVtx.covariance();
+      SMatrixSym3D totalCov = theBeamSpot->rotatedCovariance3D() + theVtx.covariance();
+      if (useVertex_) totalCov = referenceVtx.covariance() + theVtx.covariance();


same as above why twice?

brw is this fix not changing physics?
and if not changing physics, why modify?

Hey Vincenzo -

everything I changed only gets implemented if the user specifies
non-standard configuration.
This has to do with either:

using a primary vertex instead of a beam spot

using z information in calculating variables
by default, the useVertex_ flag will not be set and so this stuff doesn't
get used.

I can change the way the flags / if statements are implemented if you like

but maybe after the heavy ion run & in whatever the current CMSSW is by
then?...

On Wed, Oct 28, 2015 at 9:06 AM, Vincenzo Innocente <
[email protected]> wrote:

In RecoVertex/V0Producer/src/V0Fitter.cc
#12142 (comment):

@@ -206,7 +203,8 @@ void V0Fitter::fitAll(const edm::Event& iEvent, const edm::EventSetup& iSetup,
GlobalPoint vtxPos(theVtx.x(), theVtx.y(), theVtx.z());

// 2D decay significance

const SMatrixSym3D totalCov = theBeamSpot->rotatedCovariance3D() + theVtx.covariance();

SMatrixSym3D totalCov = theBeamSpot->rotatedCovariance3D() + theVtx.covariance();

if (useVertex_) totalCov = referenceVtx.covariance() + theVtx.covariance();

same as above why twice?

brw is this fix not changing physics?

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/12142/files#r43267634.

som why fixing 76?

i added the 3D/vertex features to 76X, 75x, 74X and for the upcoming heavy
ion run
#11660

but there was a bug, so this last round of PRs is fixing that bug in the
appropriate releases

On Wed, Oct 28, 2015 at 9:14 AM, Vincenzo Innocente <
[email protected]> wrote:

In RecoVertex/V0Producer/src/V0Fitter.cc
#12142 (comment):

@@ -206,7 +203,8 @@ void V0Fitter::fitAll(const edm::Event& iEvent, const edm::EventSetup& iSetup,
GlobalPoint vtxPos(theVtx.x(), theVtx.y(), theVtx.z());

// 2D decay significance

const SMatrixSym3D totalCov = theBeamSpot->rotatedCovariance3D() + theVtx.covariance();

SMatrixSym3D totalCov = theBeamSpot->rotatedCovariance3D() + theVtx.covariance();

if (useVertex_) totalCov = referenceVtx.covariance() + theVtx.covariance();

som why fixing 76?

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/12142/files#r43268712.

cmsbuild · 2015-10-28T17:10:37Z

+1
Tested at: 69659d3
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12142/9263/summary.html

cmsbuild · 2015-10-28T18:12:27Z

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

slava77 · 2015-10-28T22:19:10Z

+1

for #12142 69659d3

the same code change as in 75X allow vertex covariance #12133 and 74X allow user to select vtx or beamspot covariance #12097: bugfix to new option to select V0 (not used in default V0 reco)
jenkins tests pass and comparisons with baseline show no differences (expected, since the new option is not executed)

cmsbuild · 2015-10-28T22:21:39Z

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

davidlange6 · 2015-10-29T11:00:06Z

+1

add vertex covariance for V0Producer 76x

add vertex covariance for V0Producer 76x

69659d3

cmsbuild added this to the Next CMSSW_7_6_X milestone Oct 28, 2015

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Oct 28, 2015

VinInn reviewed Oct 28, 2015
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Oct 28, 2015

cmsbuild added tests-approved and removed tests-started labels Oct 28, 2015

cmsbuild added comparison-available and removed comparison-pending labels Oct 28, 2015

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Oct 28, 2015

cmsbuild added orp-approved and removed orp-pending labels Oct 29, 2015

cmsbuild added a commit that referenced this pull request Oct 29, 2015

Merge pull request #12142 from fojensen/mydev_760p7

d8345fe

add vertex covariance for V0Producer 76x

cmsbuild merged commit d8345fe into cms-sw:CMSSW_7_6_X Oct 29, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add vertex covariance for V0Producer 76x #12142

add vertex covariance for V0Producer 76x #12142

fojensen commented Oct 28, 2015

cmsbuild commented Oct 28, 2015

slava77 commented Oct 28, 2015

VinInn Oct 28, 2015

slava77 Oct 28, 2015

cmsbuild commented Oct 28, 2015

VinInn Oct 28, 2015

fojensen Oct 28, 2015

VinInn Oct 28, 2015

fojensen Oct 28, 2015

cmsbuild commented Oct 28, 2015

cmsbuild commented Oct 28, 2015

slava77 commented Oct 28, 2015

cmsbuild commented Oct 28, 2015

davidlange6 commented Oct 29, 2015

add vertex covariance for V0Producer 76x #12142

add vertex covariance for V0Producer 76x #12142

Conversation

fojensen commented Oct 28, 2015

cmsbuild commented Oct 28, 2015

slava77 commented Oct 28, 2015

VinInn Oct 28, 2015

Choose a reason for hiding this comment

slava77 Oct 28, 2015

Choose a reason for hiding this comment

cmsbuild commented Oct 28, 2015

VinInn Oct 28, 2015

Choose a reason for hiding this comment

fojensen Oct 28, 2015

Choose a reason for hiding this comment

VinInn Oct 28, 2015

Choose a reason for hiding this comment

fojensen Oct 28, 2015

Choose a reason for hiding this comment

cmsbuild commented Oct 28, 2015

cmsbuild commented Oct 28, 2015

slava77 commented Oct 28, 2015

cmsbuild commented Oct 28, 2015

davidlange6 commented Oct 29, 2015