CSC segment fit factored and smatrixed #7864
Conversation
|
A new Pull Request was created by @ptcox for CMSSW_7_5_X. CSC segment fit factored and smatrixed It involves the following packages: RecoLocalMuon/CSCSegment @cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
comparison-pending
orp-pending
and removed
comparison-pending
orp-pending
pending-signatures
reconstruction-pending
tests-pending
labels
|
.. I'm checking it |
| float Rdev( float x, float y, float z ) const; | ||
|
|
||
| // Other public functions are accessors | ||
| CSCSetOfHits hits(void) const { return hits_; } |
There was a problem hiding this comment.
why not return a const CSCSetOfHits& ?
There was a problem hiding this comment.
Hi Slava,
The original code in several of the algorithms uses the set of hits as a cache and often manipulates it, inserting or erasing elements. My simple approach was just to make sure the CSCSegFit could satisfy their requirements, so I wanted a copy of the initial set of hits held in the CSCSegFit object.
I am not sure the existing code will work even with a reference, let alone a const reference. Maybe it can be changed, but that is an optimization for the future, not a show stopper, and somebody would have to ensure everything continues to work fine.
On a side note, Piet and I were discussing earlier in the week how to adapt this for GEM too, since the same CLHEP-based code has also been entangled in the GEM local reco. It is conceivable that the code could be extended to TrackingRecHits, and dimensions will have to be increased to at least allow 8 hits. At the same time one could attempt to clarify ownership and constness of the set of hits.
Regards, Tim
On Feb 21, 2015, at 00:05 , Slava Krutelyov notifications@github.com wrote:
In RecoLocalMuon/CSCSegment/src/CSCSegFit.h:
- // Change scale factor of rechit x error
- // - expert use only!
- void setScaleXError ( double factor ) { scaleXError_ = factor; }
- // Fit values
- float xfit( float z ) const;
- float yfit( float z ) const;
- // Deviations from fit for given input (local w.r.t. chamber)
- float xdev( float x, float z ) const;
- float ydev ( float y, float z ) const;
- float Rdev( float x, float y, float z ) const;
- // Other public functions are accessors
- CSCSetOfHits hits(void) const { return hits_; }
why not return a const CSCSetOfHits& ?
—
Reply to this email directly or view it on GitHub.
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar |
|
+1 |
CSC segment fit factored and smatrixed
This completes the conversion of CLHEP matrices to ROOT::SMatrix in the CSC least-squares fit code of all 5 CsC segment building algorithms, as required for CMSSW multithreading compatiiblity. It does this by factoring out the fit into a base class CSCSegFit, where the SMatrix code lives. The current default segment builder algorithm, CSCSegAlgoST, uses a derived class CSCCondSegFit, which involves some sophisticated (i.e. I don't understand) conditioning passes. All 4 other algorithms use the simple base class. While making this conversion and factoring I needed to bring the old algorithms back to life - none had been run in years. This involved updating their config files (adding me42). They all now run on 73X relval samples. A number of minor bugs were fixed. My basic validation was to ensure that the default ST algorithm produces the same, identical, segments in 1000 TTbar + 25ns BX pileup events from the 73X relvals. This newly-factored seg-fit version does indeed produce identical segments to those in the ST version with the SMatrix-based seg fit embedded. I have also run the same 1000 events with valgrind and there are no errors or memory leaks detected in this code (or elsewhere in CSC local reco.) Because this version should now be multithreading compatible I have once more changed the CSCSegmentProducer to a stream::EDProducer. Finally, I ran the Reco-required "runTheMatrix.py -s --useInput all" tests in a 75X IB and they completed with no errors. I have one caveat: I know nothing about 'fillDescriptions' but I get the impression that sooner or later HLT will want that included. Because there are multiple algorithms it will be complex to write such functions and I don't plan to do it any time soon (April would be a possible timescale, if I am still around.)