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

TrackingRecHit rework #2598

Merged
merged 15 commits into from Mar 5, 2014
Merged

TrackingRecHit rework #2598

merged 15 commits into from Mar 5, 2014

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Feb 23, 2014

This is the first step toward a rework of TrackingRecHit and their Transient counterparts to remove all inconsistency and eventually remove the need of two class hierarchies to represent the very same thing.

This PR includes

  1. move DetSet * in TrackingRecHit

  2. move interface to access global coordinate as well
    2a) implement it in BaseTrackerRecHit

  3. construct TrackingRecHit with a valide DetSet pointer for Tracker

  4. change the interface of MeasurementEstimator to accept TrackingRecHit
    so long not a single relevant code instruction is most probably changed...

  5. remove the need to create a TTRH just to verify that it is compatible with trajectory during matching (commit 9ef436a and b29f971)
    this avoids memory churn and the need of a thread-safe static

  6. add alignment errors to local errors directly for Tracker in BaseTrackerRecHit
    this modification will create some regression due to the fact that now Alignment errors will be visible during Seeding and PixelTrack creation as well.
    (This is the first of the serious inconsistencies that is removed)

verified running full Matrix
regression from 202.0 with MTV in https://innocent.web.cern.ch/innocent/NewTkHitMTV/
regression from jetHT 2012C in https://innocent.web.cern.ch/innocent/NewTkHit/
most "affected" should be PixelTracks

next steps (after this PR is merged)
a) fix PixelRecHit content and construction
b) use normal SiStrip2DRecHit during matching
c) make TrackingRecHit able to clone (or update?) w.r.t. new track parameters
d) move Muon as well?
e) migrate all TrackingTools and RecoTracker interfaces to use TrackingRecHit
f) clean the code


bool BaseTrackerRecHit::hasPositionAndError() const {
return (err_.xx() != 0) || (err_.yy() != 0) || (err_.xy() != 0) ||
(pos_.x() != 0) || (pos_.y() != 0) || (pos_.z() != 0);
return det();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

faster and safer!

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_1_X.

TrackingRecHit rework

It involves the following packages:

CommonTools/Utils
DataFormats/TrackerRecHit2D
DataFormats/TrackingRecHit
FastSimulation/TrajectoryManager
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaPhotonAlgos
RecoLocalTracker/SiPixelRecHits
RecoLocalTracker/SiStripRecHitConverter
RecoMuon/GlobalTrackingTools
RecoMuon/L3TrackFinder
RecoMuon/TrackingTools
RecoTracker/MeasurementDet
RecoTracker/TkTrackingRegions
RecoTracker/TrackProducer
RecoTracker/TransientTrackingRecHit
TrackingTools/DetLayers
TrackingTools/GsfTracking
TrackingTools/KalmanUpdators
TrackingTools/TransientTrackingRecHit

@thspeer, @lveldere, @monttj, @cmsbuild, @anton-a, @nclopezo, @giamman, @slava77, @vadler, @Degano can you please review it and eventually sign? Thanks.
@ghellwig, @GiacomoSguazzoni, @rovere, @gpetruc, @cerati, @bachtis 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.

void
TkGluedMeasurementDet::HitCollectorForFastMeasurements::add(SiStripMatchedRecHit2D const& hit2d)
{
hasNewHits_ = true; //FIXME: see also what happens moving this within testAndPush
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do not even try to move this after the test: fake rate explodes!

@VinInn
Copy link
Contributor Author

VinInn commented Feb 23, 2014

CommonTools/Utils/test/testCutParser.cc
is involved just because it uses SiStrip2DRecHit.
Would not be better to use a test class designed for the specific purpose?

@VinInn
Copy link
Contributor Author

VinInn commented Feb 24, 2014

Let me comment a bit more about APE:
one of course would not expect it to be applied to the LocalError.
One expect it to be applied to the GlobalError and to the predition of the trajectory state.
This is not the case in CMSSW.
GlobalError (just look everywhere ErrorFrameTransformer is used
https://cmssdt.cern.ch/SDT/lxr/ident?v=CMSSW_7_1_0_pre2;i=ErrorFrameTransformer
is just a transformation of the localError. (nothing prevents to apply there, geomdet is available..)
For what concern the trajectory it was chosen the opposite: to apply it to the measurement.
see for instance the original ORCA code in
https://cmssdt.cern.ch/SDT/lxr/source/DataFormats/TrackingRecHit/src/RecHit1D.cc?v=CMSSW_7_1_0_pre2#024
the net effect is that it is taken into account only in KF in a quite cryptic way.
All other use of TrackingHits ignores Alignment Errors... (notably PixelTracks, seeding with not matched hits and EtaPhiMeasurementEstimator)

I do understand the logic of not adding APE to the persistent local error with the idea to read persistent rechit and redo fit or similar using localpos/localerr has stored and a new alignment.
This is not anyone possible since CMSSW_2 as localpos/err are not stored anymore. So, unless somebody wants to store again pos/err,
I think it is much better, for sake of consistency and efficiency, to add APE once, in one place, even if, under strict principle, it is the wrong place.

ps.
for fastsim it is used "correctly" when creating the cluster
https://cmssdt.cern.ch/SDT/lxr/source/FastSimulation/TrackingRecHitProducer/src/SiTrackerGaussianSmearingRecHitConverter.cc?v=CMSSW_7_1_0_pre2#784

@cmsbuild
Copy link
Contributor

-1
I ran the usual tests and I found errors in the following unit tests:

---> test testCommonToolsUtil had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2598/273/summary.html

@VinInn
Copy link
Contributor Author

VinInn commented Feb 24, 2014

On 24 Feb, 2014, at 12:37 PM, cmsbuild notifications@github.com wrote:

-1
I ran the usual tests and I found errors in the following unit tests:

---> test testCommonToolsUtil had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2598/273/summary.html

aggg coming back..
I will fix.. (deleting the test alltogether as using a TrackerRecHIt improperly???

v.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2014

Pull request #2598 was updated. @thspeer, @lveldere, @monttj, @cmsbuild, @anton-a, @nclopezo, @giamman, @slava77, @vadler, @Degano can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Mar 4, 2014

+1

for #2598 3cd3ea5
based on tests of #2598 67adcf1
discussed above.

The last commit is trivial.
It makes me wonder though how did the other version with of BaseTrackerRecHit.cc with "void inline" from #2673 merged on Feb 28, available from 03-01-0200 managed to compile. is the word "inline" ignored in .cc ?

@VinInn
Copy link
Contributor Author

VinInn commented Mar 5, 2014

the usual testCommonToolsUtil seem to fail now

/bin/sh: line 1:  3575 Segmentation fault      (core dumped) ( testCommonToolsUtil ) >> tmp/slc6_amd64_gcc490/src/CommonTools/Utils/test/testCommonToolsUtil/testing.log 2>&1

it does not fail in my test area!

I run

valgrind ../test/slc6_amd64_gcc490/testCommonToolsUtil >& testCommonToolsUtil.Valgrind.log &
valgrind $CMSSW_RELEASE_BASE/test/slc6_amd64_gcc490/testCommonToolsUtil > & testCommonToolsUtil_ORI.Valgrind.log &

plenty of valgrind messages in both
none related directly to TrackingRecHit part

no obvious differences (besides the pid e library name)

sed -i 's/==10008==/==10005==/g' testCommonToolsUtil_ORI.Valgrind.log
diff testCommonToolsUtil.Valgrind.log testCommonToolsUtil_ORI.Valgrind.log 
4c4
< ==10005== Command: ../test/slc6_amd64_gcc490/testCommonToolsUtil
---
> ==10005== Command: /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil
216,217c216,217
< ==10005==    by 0x4293A2: testExpressionParser::checkTrack(std::string const&, double) (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
< ==10005==    by 0x42A6FB: testExpressionParser::checkAll() (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
---
> ==10005==    by 0x4284A2: testExpressionParser::checkTrack(std::string const&, double) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
> ==10005==    by 0x4297FB: testExpressionParser::checkAll() (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
222c222
< ==10005==  Address 0x11842d68 is 24 bytes inside a block of size 63 free'd
---
> ==10005==  Address 0xf00bbf8 is 24 bytes inside a block of size 63 free'd
231,232c231,232
< ==10005==    by 0x4293A2: testExpressionParser::checkTrack(std::string const&, double) (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
< ==10005==    by 0x42A6FB: testExpressionParser::checkAll() (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
---
> ==10005==    by 0x4284A2: testExpressionParser::checkTrack(std::string const&, double) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
> ==10005==    by 0x4297FB: testExpressionParser::checkAll() (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
243,244c243,244
< ==10005==    by 0x4293A2: testExpressionParser::checkTrack(std::string const&, double) (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
< ==10005==    by 0x42A6FB: testExpressionParser::checkAll() (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
---
> ==10005==    by 0x4284A2: testExpressionParser::checkTrack(std::string const&, double) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
> ==10005==    by 0x4297FB: testExpressionParser::checkAll() (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
249c249
< ==10005==  Address 0x11842d68 is 24 bytes inside a block of size 63 free'd
---
> ==10005==  Address 0xf00bbf8 is 24 bytes inside a block of size 63 free'd
258,259c258,259
< ==10005==    by 0x4293A2: testExpressionParser::checkTrack(std::string const&, double) (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
< ==10005==    by 0x42A6FB: testExpressionParser::checkAll() (in /home/vin/TkHit/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
---
> ==10005==    by 0x4284A2: testExpressionParser::checkTrack(std::string const&, double) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
> ==10005==    by 0x4297FB: testExpressionParser::checkAll() (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
718c718
< ==10005==   total heap usage: 4,077,802 allocs, 3,726,753 frees, 248,908,386 bytes allocated
---
> ==10005==   total heap usage: 4,077,801 allocs, 3,726,752 frees, 249,151,622 bytes allocated

@VinInn
Copy link
Contributor Author

VinInn commented Mar 5, 2014

Sorry the previous test is not fully correct as it was done using original binary and current libraries
(incredible that worked!)

again this time original in a brand new empty area...
more diffs because now the libraries are really different,
no obvious new error though

let me note that in the original report there are already errors like this one:

checking expression: "extra.outerPhi" = 1.4289
checking lazy expression: "extra.outerPhi"==10005== Invalid read of size 1
==10005==    at 0x4809F53: strcmp (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/external/valgrind/3.9.0/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10005==    by 0x85131CC: reco::typeCode(edm::TypeWithDict const&) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x84F1411: reco::parser::SingleInvoker::SingleInvoker(edm::TypeWithDict const&, std::string const&, std::vector<boost::variant<signed char, unsigned char, short, unsigned short, int, unsigned int, long, unsigned long, double, float, std::string, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_>, std::allocator<boost::variant<signed char, unsigned char, short, unsigned short, int, unsigned int, long, unsigned long, double, float, std::string, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_> > > const&) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x84F22CC: reco::parser::LazyInvoker::invoker(edm::TypeWithDict const&) const (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x84F2522: reco::parser::LazyInvoker::invokeLast(edm::ObjectWithDict const&, std::vector<edm::ObjectWithDict, std::allocator<edm::ObjectWithDict> >&) const (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x84ED716: reco::parser::ExpressionLazyVar::value(edm::ObjectWithDict const&) const (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x4284A2: testExpressionParser::checkTrack(std::string const&, double) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
==10005==    by 0x4297FB: testExpressionParser::checkAll() (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/test/slc6_amd64_gcc490/testCommonToolsUtil)
==10005==    by 0xC0E9721: CppUnit::TestCaseMethodFunctor::operator()() const (TestCase.cpp:32)

or like this other one (same location)

==10005== Invalid read of size 1
==10005==    at 0x3267F28590: __strcmp_sse42 (in /lib64/libc-2.12.so)
==10005==    by 0x85131A2: reco::typeCode(edm::TypeWithDict const&) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x84F1411: reco::parser::SingleInvoker::SingleInvoker(edm::TypeWithDict const&, std::string const&, std::vector<boost::variant<signed char, unsigned char, short, unsigned short, int, unsigned int, long, unsigned long, double, float, std::string, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_>, std::allocator<boost::variant<signed char, unsigned char, short, unsigned short, int, unsigned int, long, unsigned long, double, float, std::string, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_, boost::detail::variant::void_> > > const&) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)
==10005==    by 0x84F22CC: reco::parser::LazyInvoker::invoker(edm::TypeWithDict const&) const (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/cms/cmssw/CMSSW_7_1_X_2014-03-04-0200/lib/slc6_amd64_gcc490/libCommonToolsUtils.so)

or (several locations)

checking lazy expression: "numberOfDaughters"==10005== Conditional jump or move depends on uninitialised value(s)
==10005==    at 0xAE8C5E2: TObject::~TObject() (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/lcg/root/5.34.10-cms/lib/libCore.so)
==10005==    by 0xAF2864F: ROOT::TSchemaRule::ProcessList(TObjArray*, TString const&) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/lcg/root/5.34.10-cms/lib/libCore.so)
==10005==    by 0xAF1E16F: ROOT::TGenericClassInfo::CreateRuleSet(std::vector<ROOT::TSchemaHelper, std::allocator<ROOT::TSchemaHelper> >&, bool) (in /afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc490/lcg/root/5.34.10-cms/lib/libCore.so)

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2014

@ktf
Copy link
Contributor

ktf commented Mar 5, 2014

Merging this, bypassing Analysis (@volker are you there?) and Fastsim (since @lveldere already signed it).

ktf added a commit that referenced this pull request Mar 5, 2014
Reco updates -- TrackingRecHit rework
@ktf ktf merged commit f3b076f into cms-sw:CMSSW_7_1_X Mar 5, 2014
@ktf
Copy link
Contributor

ktf commented Mar 5, 2014

@nclopezo can you make sure tests do not run if there are conflicts???

@VinInn
Copy link
Contributor Author

VinInn commented Mar 5, 2014

forget about all the noise above...
eventually testCommonToolsUtil successes in
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2598/353/unitTests.log

@nclopezo
Copy link
Contributor

nclopezo commented Mar 5, 2014

@ktf The tests never run when there are conflicts, that makes the jenkins job to fail. I always start then when the pull request is mergeable, in this case it should have become unmergeable after the tests were published.

@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre5, CMSSW_7_1_0_pre4 Mar 10, 2014
@cmsbuild
Copy link
Contributor

ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
@VinInn VinInn deleted the TkHit branch April 21, 2017 11:47
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