Move jacobian calculation to DF

[arizzi]

This PR moves the simple jacobian matrix definition from TrackingTools to DataFormats.
With this change we can properly compute the uncertainty on sum of Candidates as needed by btag for VertexCompositePtrCandidate.
(The fix for VertexCompositePtrCandidate wrt original Dinko's implementation will be in a separate later PR)

@ferencek @VinInn

[VinInn]

+1

[cmsbuild]

A new Pull Request was created by @arizzi for CMSSW_7_3_X.

Move jacobian calculation to DF

It involves the following packages:

DataFormats/GeometryVector
TrackingTools/AnalyticalJacobians

@civanch, @nclopezo, @mdhildreth, @cmsbuild, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @gpetruc, @cerati, @venturia 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[slava77]

If we move, we should move all of the Jacobians, not cherry-pick on one use-case basis.
This doesn't provide a persisted class (the main purpose of DataFormats), we should try to keep plain math in one place, like DataFormats/Math.
@Dr15Jones please advise, the rules on DF dependencies are in your court.

It makes sense to me to have this and similar other somewhat "basic" plain math available in FWLite (hence, the choice of using the DataFormats subsystem as a historical choice is OK).
However, I don't like to allow this as an entry point to let regular persisted classes to call Jacobians internally and incur cost of calling 6x6 matrix multiplication operations in streamers or "naively" simple (like Candidate::track) accessor methods of persisted types.

[VinInn]

there is dependency on GeometryVector, so this is the lowest level place where they can enter.
Traditionally any utility required by a DataFormat needs to be stay in DataFormat (even more if depends on DataFormat)
I remind that we need to reduce the number of libraries. merging Math GeometryVector and GeometrySurface: I am favorable provided we find who does it.

[slava77]

If it's done with XYZVectorF, the code can stay in DataFormats/Math

[VinInn]

following such line of though we can get rid of GeometryVector alltogether (not necessary a bad idea)

[arizzi]

of course it can be done replacing GlobalVector with math::GlobalVector,
but then it is no more a code move and someone should validate that no
regression is introduced by differences in the implementations.

On Thu, Oct 9, 2014 at 1:00 PM, Slava Krutelyov notifications@github.com
wrote:

If it's done with XYZVectorF, the code can stay in DataFormats/Math

—
Reply to this email directly or view it on GitHub
#5753 (comment).

[VinInn]

in any case I will veto any use of Root vectors in Tracking, so please do not start such a discussion

[cmsbuild]

+1
Tested at: a26c8c9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5753/230/summary.html

[cmsbuild]

+1
Tested at: a26c8c9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5753/231/summary.html

[slava77]

Is the cost of an extra instance of a 6x5 matrix local + copy to return by value negligible enough?

[VinInn]

given the presence of R and of theJacobian * R;
most probably yes...
Of course we should remove the use of the old class and use the function in the few places where is used..
Cartesian errors are not used in Tracking... only in vertex fitting
https://cmssdt.cern.ch/SDT/lxr/source/RecoVertex/KinematicFitPrimitives/src/TrackKinematicStatePropagator.cc?v=CMSSW_7_2_0_pre7#0247
the only instance

[VinInn]

here is a complete list
https://cmssdt.cern.ch/SDT/lxr/ident?v=CMSSW_7_2_0_pre7&_i=cartesianError
and infact there are two places where cartesianError() is invoked in tracking just to get czz.
to be fixed...

[cmsbuild]

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

[cmsbuild]

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

[civanch]

+1

[arizzi]

do we understand why the time increases?

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

[slava77]

A few more details on the increase in CPU:

There is an extra instance of a matrix (with a copy) in the code compared to the old implementation, but this appears to be a sub-dominant effect (at least this part is not showing up explicitly in the breakdown of calls in the igprof).
The cost of sincos and atan2f (about 40% of the JacobianCurvilinearToCartesian call), actually increased proportionally.
I'm guessing there's something to do with cache use.

Functions not using the jacobian have the same cost in my reference and "new" runs. So, I think this increase is more likely to be real.

[slava77]

@ktf @nclopezo
Giulio and David,
any idea why the IB summary page has this text
#5753 from arizzi: Utilities/ReleaseScripts -- Move jacobian calculation to DF
Why Utilities/ReleaseScripts ?

[nclopezo]

Hi Slava,
It was by bad, I accidentally pasted the "Utilities/ReleaseScripts" when I merged it.