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.

Sign up for GitHub

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

Jump to bottom

RecoLocalTracker/SiStripRecHitConverter: Fix bug found by clang warningand fix clang warnings: #22533

Merged
merged 1 commit into from Mar 10, 2018
Merged

RecoLocalTracker/SiStripRecHitConverter: Fix bug found by clang warningand fix clang warnings: #22533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions RecoLocalTracker/SiStripRecHitConverter/interface/StripCPEfromTemplate.h
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ class StripCPEfromTemplate : public StripCPE

public:

using StripCPE::localParameters;

StripClusterParameterEstimator::LocalValues
localParameters( const SiStripCluster&, const GeomDetUnit&, const LocalTrajectoryParameters&) const override;
Expand Down
3 changes: 2 additions & 1 deletion RecoLocalTracker/SiStripRecHitConverter/interface/StripCPEgeometric.h
View file
Open in desktop
Expand Up @@ -8,7 +8,8 @@ class StripCPEgeometric : public StripCPE
{

public:


using StripCPE::localParameters;
StripClusterParameterEstimator::LocalValues
localParameters( const SiStripCluster&, const GeomDetUnit&, const LocalTrajectoryParameters&) const override;

Expand Down
2 changes: 1 addition & 1 deletion RecoLocalTracker/SiStripRecHitConverter/src/SiStripTemplate.cc
View file
Open in desktop
Expand Up @@ -552,7 +552,7 @@ bool SiStripTemplate::pushfile(const SiPixelTemplateDBObject& dbobject, std::vec
qavg_avg = 0.f;
for (j=0; j<9; ++j) {

for (l=0; l<TSXSIZE; ++k) {db >> theCurrentTemp.entx[k][i].xtemp[j][l]; qavg_avg += theCurrentTemp.entx[k][i].xtemp[j][l];}
for (l=0; l<TSXSIZE; ++l) {db >> theCurrentTemp.entx[k][i].xtemp[j][l]; qavg_avg += theCurrentTemp.entx[k][i].xtemp[j][l];}
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix, while due, seems to possible affect the normal workflow of the code: when k was (erroneously) incremented, the wrong k entered the following checks inside the outer loop
@OlivierBondu : whenever there are no issues with what stored in the the db probably the previous bug didn't show up, Do you expect situations in which there is any effect of this fix? Where should we look at to possibly see one?

Copy link
Contributor

Choose a reason for hiding this comment

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

there is NOT such a thing as a StripTemplate in the database....

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that this part of the code was never hit, and therefore while the fix is due we cannot see any effect out of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Naively thought the effect of the bug should be an infinite loop (likely leading to a segfault at some point).

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed... That's correct @makortel: this demonstrates that this code was never run!

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a quick look, and it seems there are two reasons why this did not give problems:

  • the template CPE is not used for the strips (StripCPEfromTrackAngle is the default), and the template cfi is only imported in some outdated test configs
  • there is no code in CMSSW that calls this method (the template CPE uses the one defined above, bool SiStripTemplate::pushfile(int filenum, std::vector< SiStripTemplateStore > & theStripTemp_)) - I verified by removing it and recompiling (after git cms-checkdeps -a), which showed no problems.

According to git this has not been touched since the migration from CVS. I think it has been copied/adapted from the corresponding code for the pixel detector here at some point.
Question (also to @mmusich @echabert @boudoul ): since this is not used (and apparently never has been), and it can be revived from git history or the pixel version in case it is needed some day, should we just remove this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, @pieterdavid, @perrotta,
as you correctly point out Strip template reconstruction was never developed (same people working on the pixel one were involved here) up to a point where it could be used in production and certainly we don't even have CondFormat to be able to store in DB. While we need to support the fromTrackAngle and, up to a certain degree the geometric CPE algorithm, I personally see no reason to keep in repository the template code at all if all it does is to create maintenance overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second @mmusich 's comment

Copy link
Contributor

Choose a reason for hiding this comment

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

@pieterdavid @mmusich @echabert @boudoul: will any of you take care of preparing a PR which removes from the release whatever is not used (in this package)? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mmusich and @boudoul , I was only talking about this specific method, but I'm not against removing it completely either... @perrotta : submitted as #22546


if(db.fail()) {LOGERROR("SiStripTemplate") << "Error reading file 20, no template load, run # " << theCurrentTemp.entx[k][i].runnum << ENDL; return false;}
}
Expand Down