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

Bug fix for GED electron MVA preselection calculator #2558

Merged
merged 5 commits into from
Mar 11, 2014

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Feb 20, 2014

Properly initialize and use some variables that were missed in the calculation of the electron preselection-mva.

These variables were present in the training, but not in the evaluation, so they carry some rejection power.

Small expected changes for tracker-driven only electrons, small improvements in efficiency/fake-rate.

Now includes #2717 and #2719 .

We should backport this to 700.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_1_X.

Bug fix for GED electron MVA preselection calculator

It involves the following packages:

RecoEgamma/ElectronIdentification

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

@slava77
Copy link
Contributor

slava77 commented Feb 20, 2014

Hi Lindsey, could you please post some comparison plots describing the changes here so that we roughly know what to expect in testing and, maybe more importantly, to have a reference for the release history

Thanks.

@lgray
Copy link
Contributor Author

lgray commented Feb 20, 2014

Hi Slava,
I'll post some of the plots from Simon here in a bit.
There's also going to be a separate PR for an(other) update to the preselection MVA.
Simon was able to improve the size and composition of the training sample.

@cmsbuild
Copy link
Contributor

@dbenedet
Copy link
Contributor

Hi Slava, here the plots from Simon. The fake rate increase in the endcap but it is still below 1% that is our goal. At the same time we have un increase of efficiency.
fakerateplot
efficiencyplot

@dbenedet
Copy link
Contributor

@lgray we would need as usual to update the weight file in a second moment. Thanks.

@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre4, CMSSW_7_1_0_pre3 Feb 24, 2014
@slava77
Copy link
Contributor

slava77 commented Feb 27, 2014

Hi Daniele and Lindsey, please clarify on your comment about the weight file.
Were your plots made with the weight file updated or not?

The file is in the external: was the request for an update submitted?
Once it's updated, this PR would need to have a new download.url, I guess, unless you stay with the old name.

Thanks

@dbenedet
Copy link
Contributor

Hi Slava,

yes the plots are done with the new weight file. However it change very
little, with respect to the old trained file.
The external file is available in the usual /afs directory but I think
that still it needs to be requested.
When it will be available we will update the download.url and the
corresponding python files.

cheers,
Daniele

On 27/02/14 01:07, slava77 wrote:

Hi Daniele and Lindsey, please clarify on your comment about the
weight file.
Were your plots made with the weight file updated or not?

The file is in the external: was the request for an update submitted?
Once it's updated, this PR would need to have a new download.url, I
guess, unless you stay with the old name.

Thanks


Reply to this email directly or view it on GitHub
#2558 (comment).

@slava77
Copy link
Contributor

slava77 commented Feb 27, 2014

Hi Daniele,
Since the external file and download.url update will need another check, I just wanted to reduce the number of incremental epsilon tests and do it in one go.
What's the request number in cmsdist for this file update?
Please let me know

@slava77
Copy link
Contributor

slava77 commented Mar 3, 2014

@lgray @dbenedet
Daniele and Lindsey, what's the status with the external update with the new weight file?

@slava77
Copy link
Contributor

slava77 commented Mar 4, 2014

Hi Lindsey

Once cms-sw/cmsdist#465 is resolved and is in the IB,
I'd rather see all changes from #2717 and #2719 included here and the other two PRs closed.

Giulio, is it too late for 05-0200 IB?

@lgray
Copy link
Contributor Author

lgray commented Mar 4, 2014

Hi Slava,
No problem, once it's included I'll toss the other two commits in here.

@ktf
Copy link
Contributor

ktf commented Mar 5, 2014

It was too late for me ;-) @Degano can you update the files? Thanks.

@lgray
Copy link
Contributor Author

lgray commented Mar 5, 2014

@ktf @Degano Please let me know which IB the updated payload will go in, thanks!

@ghost
Copy link

ghost commented Mar 5, 2014

Hi,
I've updated the files in cms-sw/cmsdist@7f4cfab
And merged the PR already, so it should go in this afternoon IB (1400).

@slava77
Copy link
Contributor

slava77 commented Mar 5, 2014

Great. Thanks, Alessandro.
Lindsey, you can update this already (no need to wait until ~6-8PM for this to actually show up in scram)

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2014

Pull request #2558 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

@nclopezo
Copy link
Contributor

nclopezo commented Mar 6, 2014

@lgray
Hi Lindsey, this pull request has become unmergeable, could you please rebase it?

@lgray
Copy link
Contributor Author

lgray commented Mar 6, 2014

@nclopezo now based on CMSSW_7_1_X_2014-03-05-0200

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2014

Pull request #2558 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

@ktf
Copy link
Contributor

ktf commented Mar 6, 2014

@Degano the changes you made broke the IB. Please add back the files you removed (creating a new cms-data). We should remove files only when they are not used anymore.

@nclopezo
Copy link
Contributor

nclopezo commented Mar 7, 2014

@slava77
Copy link
Contributor

slava77 commented Mar 7, 2014

checking it now

@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre5, CMSSW_7_1_0_pre4 Mar 10, 2014
@slava77
Copy link
Contributor

slava77 commented Mar 10, 2014

+1

for #2558 2192eff
Changes as expected: all diffs start at the MVA (it peaks better at 1 for electron guns).

  • Some increase in ged electrons in the endcaps in ele gun sample
  • Around 10% increase in ged electron counts in the jetty/background-like samples.

@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?

nclopezo added a commit that referenced this pull request Mar 11, 2014
RecoEgamma/ElectronIdentification -- Bug fix for GED electron MVA preselection calculator
@nclopezo nclopezo merged commit 9b6d898 into cms-sw:CMSSW_7_1_X Mar 11, 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

7 participants