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

remove unused variable #4140

Merged

Conversation

pvmulder
Copy link
Contributor

@pvmulder pvmulder commented Jun 6, 2014

unused variable removed
does not affect standard reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2014

A new Pull Request was created by @pvmulder (Petra Van Mulders) for CMSSW_7_1_X.

remove unused variable

It involves the following packages:

RecoBTag/SecondaryVertex

@nclopezo, @cmsbuild, @Degano, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@ferencek 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.

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

@pvmulder , in what sense is the variable not used, isn't it upto the analysis code? How was it decided the variable would not be needed again and why is this considered a bugfix (to put in 71X at this stage)?

@pvmulder
Copy link
Contributor Author

No, it is not up to the analysis code. This is a new b-tagger that is under development and it is up to the developer which variables are used. Actually the leptonP0Par was not filled properly (it is empty) and was already removed for the muons, but I forgot to remove it for the electrons. In that sense it is a bug fix.

@StoyanStoynev
Copy link
Contributor

OK, do I get it right that other variables (like leptonRatioRel ) are YET to be used in the code - for now they are just inserted in a TaggingVariableList?

@pvmulder
Copy link
Contributor Author

That is correct in the sense that the CombinedSVSoftLeptonComputer.cc is not included in the standard reconstruction sequence. So this fix only matters to developers and not to users.

@StoyanStoynev
Copy link
Contributor

+1
Apparently jenkins comparisons will run forever...
Tested 2ca50cf with CMSSW_7_1_X_2014-06-13-0200.
I ran short matrix tests and checked results from FWlite and DQM scripts (plus the part from jenkins tests available). No diffs, see also #4139.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

davidlange6 added a commit that referenced this pull request Jun 15, 2014
@davidlange6 davidlange6 merged commit 42415bc into cms-sw:CMSSW_7_1_X Jun 15, 2014
@ferencek ferencek deleted the CSVSLbugfix_from_CMSSW_71X branch June 15, 2014 15:48
@ktf ktf mentioned this pull request Jun 23, 2014
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

5 participants