self-subtractions : possible bugs

[slava77]

as a follow up to #26868 (comment)
I made some egrep of the CMSSW code base for repeated patterns [1].

This is not exhaustive (e.g. function calls with arguments like x(y) - x(y) are not covered) and perhaps a better tool/parsing should be added.
@fabiocos @smuzaffar perhaps we even have something automated? Clang static analyzer report does not pick up these as issues.

The results are as follows:

• RecoLocalMuon/GEMSegment/plugins/ME0SegAlgoRU.cc already reported

  cmssw/RecoLocalMuon/GEMSegment/plugins/ME0SegAlgoRU.cc

  Lines 325 to 327 in ba500c0

  	bool ME0SegAlgoRU::areHitsCloseInEta(const float maxETA, const bool beamConst, const GlobalPoint& h1, const GlobalPoint& h2) const {
  	float diff = 0;
  	diff = std::fabs(h1.eta() - h1.eta());

  • @jshlee please check
• RecoPixelVertexing/PixelLowPtUtilities/src/ValidHitPairFilter.cc

  cmssw/RecoPixelVertexing/PixelLowPtUtilities/src/ValidHitPairFilter.cc

  Lines 220 to 224 in ba500c0

  	vector<float>::const_iterator iph = lower_bound(phB.begin(),phB.end(), ph);
  	if(iph > phB.begin()) phVec.push_back(iph - phB.begin() - 1);
  	else phVec.push_back(phB.end() - phB.begin() - 1);
  	if(iph < phB.end() ) phVec.push_back(iph - phB.begin() );
  	else phVec.push_back(phB.begin() - phB.begin() );

  • @makortel @VinInn please check
  • this was "fixed" in remove unused /ValidHitPairFilter code  #26972
• RecoBTag/ImpactParameter/plugins/IPProducer.h

  cmssw/RecoBTag/ImpactParameter/plugins/IPProducer.h

  Lines 393 to 398 in ba500c0

  	} else {
  	trackIP.distanceToGhostTrack = Measurement1D(-1. -1.);
  	trackIP.ghostTrackWeight = 0.;
  	}
  	} else {
  	trackIP.distanceToGhostTrack = Measurement1D(-1. -1.);

  • it looks like a comma is missing. @arizzi @ferencek please check for b-tag needs.
  • this was fixed in correct missing commas in dflt initializations for ghostTrack in IPProducer #27000
• in Fireworks/Calo/src/FWTauProxyBuilderBase.cc

  cmssw/Fireworks/Calo/src/FWTauProxyBuilderBase.cc

  Lines 95 to 99 in ba500c0

  	std::pair<double,double> phiRange = fireworks::getPhiRange( m_phis, phi );
  	double min_phi = phiRange.first-M_PI/36/2;
  	double max_phi = phiRange.second+M_PI/36/2;
  	if( fabs(phiRange.first-phiRange.first)<1e-3 ) {
  	min_phi = phi-M_PI/36/2;

  • perhaps should be first-second? @alja please check
  • this was fixed in Restore PR 27001 #27009

[1] egrep -R "(\b[a-zA-Z0-9\.]+\b)()[^A-Za-z=0-9,/:-]*-[^A-Za-z0-9>=%($]*\1[^_A-Za-z0-9]|(\b[a-zA-Z0-9\.]+\b)[^A-Za-z=0-9,/:)]*-[^A-Za-z0-9>=%($]*\1[^_A-Za-z0-9]"

[cmsbuild]

A new Issue was created by @slava77 Slava Krutelyov.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

RecoPixelVertexing/PixelLowPtUtilities/src/ValidHitPairFilter.cc

cmssw/RecoPixelVertexing/PixelLowPtUtilities/src/ValidHitPairFilter.cc

Lines 220 to 224 in ba500c0

	vector<float>::const_iterator iph = lower_bound(phB.begin(),phB.end(), ph);
	if(iph > phB.begin()) phVec.push_back(iph - phB.begin() - 1);
	else phVec.push_back(phB.end() - phB.begin() - 1);
	if(iph < phB.end() ) phVec.push_back(iph - phB.begin() );
	else phVec.push_back(phB.begin() - phB.begin() );

On a quick look this class is a good candidate for dead code (not used anywhere in CMSSW, last non-technical-migration change was in 2010 (in 2008 for this specific fragment).

[ferencek]

* [ ]  RecoBTag/ImpactParameter/plugins/IPProducer.h https://github.com/cms-sw/cmssw/blob/ba500c0d38075ac562531832c437568ab6e2dfc0/RecoBTag/ImpactParameter/plugins/IPProducer.h#L393-L398
  
  
  * it looks like a comma is missing. @arizzi @ferencek  please check for b-tag needs.

Hi @slava77, indeed, this looks like a bug and a comma is missing there. The ghost track feature is not really in active use which probably explains why this went unnoticed all this time.

[slava77]

* [ ]  RecoBTag/ImpactParameter/plugins/IPProducer.h https://github.com/cms-sw/cmssw/blob/ba500c0d38075ac562531832c437568ab6e2dfc0/RecoBTag/ImpactParameter/plugins/IPProducer.h#L393-L398
  
  
  * it looks like a comma is missing. @arizzi @ferencek  please check for b-tag needs.

Hi @slava77, indeed, this looks like a bug and a comma is missing there. The ghost track feature is not really in active use which probably explains why this went unnoticed all this time.

What would be the right fix?
It seems like the error = 0 (current incidental default) has a special meaning.
Grepping for similar calls, I see other defaults like Measurement1D(-1, 1) in

cmssw/RecoBTag/SecondaryVertex/src/GhostTrackComputer.cc

Lines 78 to 85 in ba500c0

	static const btag::TrackIPData dummy = {
	GlobalPoint(),
	GlobalPoint(),
	Measurement1D(-1.0, 1.0),
	Measurement1D(-1.0, 1.0),
	Measurement1D(-1.0, 1.0),
	Measurement1D(-1.0, 1.0),
	0.

Would this be a more correct value than what will be from a simple addition of a comma Measurement1D(-1, -1)?

[ferencek]

Measurement1D is used to store a track impact parameter and related uncertainty. The impact parameter can be signed so generally speaking Measurement1D(-1.0, 1.0) could be a valid entry. Measurement1D(-1., -1.) is clearly an invalid entry because of the negative error but the problem with this is that Measurement1D::significance() would return 1 which could look completely legitimate. Personally, I would prefer Measurement1D(0., -1.).

[alja]

@slava77 The phi range check in the FWTauProxyBuilderBase.cc is not necessary. I have submitted #27001 for this.

[slava77]

Currently, only the issue in RecoLocalMuon/GEMSegment/plugins/ME0SegAlgoRU.cc is remaining.

@nickmccoll perhaps you know what would be a correct fix here

cmssw/RecoLocalMuon/GEMSegment/plugins/ME0SegAlgoRU.cc

Lines 325 to 327 in ba500c0

	bool ME0SegAlgoRU::areHitsCloseInEta(const float maxETA, const bool beamConst, const GlobalPoint& h1, const GlobalPoint& h2) const {
	float diff = 0;
	diff = std::fabs(h1.eta() - h1.eta());

[perrotta]

Currently, only the issue in RecoLocalMuon/GEMSegment/plugins/ME0SegAlgoRU.cc is remaining.
@nickmccoll perhaps you know what would be a correct fix here

Well, the correct fix is obvious, that is replace one of the h1 with h2
What has actually to be checked before implementing it blindly is to asses its effect on the physics performance, which will likely be touched because this areHitsCloseInEta is used in the segment building. Here some hint or investigations from the GEM local reco experts is needed.

[jshlee]

Currently, only the issue in RecoLocalMuon/GEMSegment/plugins/ME0SegAlgoRU.cc is remaining.
@nickmccoll perhaps you know what would be a correct fix here

Well, the correct fix is obvious, that is replace one of the h1 with h2
What has actually to be checked before implementing it blindly is to asses its effect on the physics performance, which will likely be touched because this areHitsCloseInEta is used in the segment building. Here some hint or investigations from the GEM local reco experts is needed.

yes it should be the diff of h1 and h2, we will look into the effects on segment reco.

[nickmccoll]

@slava77 wow, that is a bad bug on my part! This is a part of the seeding, and is effectively a vertex constraint that was supposed to be, but apparently not, implemented. I would think that fixing it should just reduce the background rate (e.g. wide angle punch throughs), but it should be double checked.

[slava77]

@nickmccoll @jshlee please update on the status of checking/applying the fix in ME0SegAlgoRU.cc.
Thank you.

[slava77]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

[jshlee]

@slava77 - will update now

[perrotta]

+1

• All cases look like as having been fixed now

[cmsbuild]

This issue is fully signed and ready to be closed.