[Calibration/TkAlCaRecoProducers] [Alignment/CommonAlignmentProducer] Fix warning found by gcc 5.3.0

[ghost]

https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_8_1_X_2016-03-02-1100/Calibration/TkAlCaRecoProducers

warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare]
        if (seedOnlyFromAbove_ == 2 && thishit == 1 && type == int(StripSubdetector::TIB)) return false;

https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_8_1_X_2016-03-02-1100/Alignment/CommonAlignmentProducer

warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare]
        || applyIsolation_ || (seedOnlyFromAbove_ == 1 || seedOnlyFromAbove_ == 2)
warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare]
        if (seedOnlyFromAbove_ == 2 && thishit == 1 && subdetId == int(SiStripDetId::TIB)) {

As the intended behavior is not clear from the context.

[cmsbuild]

A new Issue was created by @Degano (Alessandro Degano).

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

cms-bot commands are list here #13029

[ghost]

assign alca

[cmsbuild]

New categories assigned: alca

@diguida,@mmusich,@cerminar,@franzoni you have been requested to review this Pull request/Issue and eventually sign? Thanks

[mmusich]

we know this one:
@mschrode @tlampen can you please follow up.

[ghellwig]

Hi all,
I had a quick look into that and according to the comment in [1], I guess this variable is wrongly declared as bool and should be an integer instead, i.e. in [2] we should declare =seedOnlyFromAbove_= as =int=.

However, I do not fully understand what the use case of this configuration parameter is...

Cheers,
Gregor

[1]
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/Alignment/CommonAlignmentProducer/src/AlignmentTrackSelector.cc#L370-L371

[2]
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/Alignment/CommonAlignmentProducer/interface/AlignmentTrackSelector.h#L71

[mschrode]

@ghellwig Thanks Gregor for looking into this!

Does any body know what the exact purpose of the seedOnlyFromAbove variable is and what the impact will be declaring it as int?

[mschrode]

As Gregor pointed out, the default setting of seedOnlyFromAbove is 0, so setting it to int will not change the default behaviour of the code. Are there objections against turning it into an int?

[mmusich]

@mschrode @ghellwig thanks for checking.
Seems that nobody is using this functionality in CMSSW:
https://github.com/cms-sw/cmssw/search?utf8=%E2%9C%93&q=seedOnlyFrom
being the seedOnlyFrom parameter always set to 0.
I guess that in the original intentions of the author, this parameter was meant to select hits only from particular track topologies (see comment https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/Alignment/CommonAlignmentProducer/src/AlignmentTrackSelector.cc#L370-L371)

// seedOnlyFrom = 1 is TIB-TOB-TEC tracks only
// seedOnlyFrom = 2 is TOB-TEC tracks only 

but seems that this was never actually used.
@ghellwig can you please propose a fix declaring seedOnlyFromAbove as int?

[davidlt]

According to git these lines predates 2013 (git migration). In that case this has not been utilised for years. Then I would just suggest to remove the feature, which makes code cleaner and easier to maintain in the future.

[ghellwig]

I have contacted the original author (Gero Flucke) of the commit introducing this feature [1](I got it from the 62X branch). I would not like to remove it without at least asking for its use case. Typically he answers within 1 or 2 days so there's not much of a delay.

[1] 00531b2

[ghellwig]

Posting Gero's reply here. @rcastello, can you comment?

Hi Gregor,
if I remember correctly, this flag was introduced by Roberto Castello at CRUZET/CRACFT or even TIF times...

I never really tried to dig into that - not sure whether it matters for any cosmics data processing nowadays. I guess not, but I did not look further (i.e. into the code itself).

Hope this helps

Gero

[mmusich]

@ghellwig I think that implementing what I suggested in #13578 (comment) is safe enough without digging too much in the past

[ghellwig]

Ok, I'll create the PRs later today.

[ghellwig]

@Degano, this issue has been fixed with #13643 and can be closed.

[mmusich]

+1

[cmsbuild]

This issue is fully signed and ready to be closed.