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

Backport 71x fixes to 70x branch #3139

Closed
wants to merge 4 commits into from

Conversation

jpavel
Copy link
Contributor

@jpavel jpavel commented Apr 1, 2014

Backporting tau bugfixes made since CMSSW_7_0_0 that are already included in 71x. See #2449 and #2843. This configuration has also been checked on Fall13 samples (https://indico.cern.ch/event/307553/contribution/3/material/slides/0.pdf) and behaviour is as expected.

The quality cuts update from #2843 (that improved performance) is not included, but it can be included as well, if you would like to speed-up the RECO sequence.

@slava77, @davidlange6, sorry for not presenting this proposal at RECO meeting, but last week was I think cancelled, and next one is on Thursday. As a result, we are open to discussion of what should be contained in this backport.

@monicava, this is something you want to watch as well

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 1, 2014

A new Pull Request was created by @jpavel (Pavel Jez) for CMSSW_7_0_X.

Backport 71x fixes to 70x branch

It involves the following packages:

RecoTauTag/ImpactParameter
RecoTauTag/RecoTau

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 1, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 1, 2014

@slava77
Copy link
Contributor

slava77 commented Apr 1, 2014

Hi Pavel,

do we have relval outputs in a 71X prerelease for all fixes proposed here?
Were they signed off by the validators based on those samples?
(That's the normal procedure for backporting)

@jpavel
Copy link
Contributor Author

jpavel commented Apr 1, 2014

Hi Slava,

the pull request 2449 was validated in 7_1_0_pre4:

https://hypernews.cern.ch/HyperNews/CMS/get/relval/2924/13.html

The pull request 2843 was included in pre5 that have not been validated
yet and no call for validation has been issued so far, as far as I can see.

On 01/04/14 18:05, slava77 wrote:

Hi Pavel,

do we have relval outputs in a 71X prerelease for all fixes proposed here?
Were they signed off by the validators based on those samples?
(That's the normal procedure for backporting)


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

Dr. Pavel JEZ
postdoctoral researcher
Centre for Cosmology, Particle Physics and Phenomenology-CP3
Université Catholique de Louvain

@slava77
Copy link
Contributor

slava77 commented Apr 1, 2014

was the combination of commits here somewhat automated to what was changed in #2449 and #2843 (rebase or cherry-pick) or was there some manual editing?

@jpavel
Copy link
Contributor Author

jpavel commented Apr 1, 2014

with one exception (printout at ...AgainstMuon2.cc) it was cherry-picking.

@jpavel
Copy link
Contributor Author

jpavel commented Apr 1, 2014

I did a diff to head of 71x version and it was identical except the changes introduced in PR's #3105, #3062, and commits b87f713 (consumes for HLT), ca398bb, 2dcf73f, 6dc9476, 8a9a199 (removing couts) f831118, 489e8b1 (changing quality cut behaviour) and f3f6a5d (clean -Wunused-function warnings)

@slava77
Copy link
Contributor

slava77 commented Apr 1, 2014

strictly speaking, at least PFRecoTauDecayModeIndexProducer is a new feature.
Is it running now?
Please remind me which new products and modules (if any) are added by this PR in the standard sequences

@jpavel
Copy link
Contributor Author

jpavel commented Apr 1, 2014

I don't understand, PFRecoTauDecayModeIndexProducer is included in 7_1_X in the same form as in this PR: https://github.com/cms-sw/cmssw/blob/CMSSW_7_1_X/RecoTauTag/RecoTau/plugins/PFRecoTauDecayModeIndexProducer.cc

Regarding the other question, there are no new products. New module is PFRecoTauDecayModeIndexProducer, that is supposed to be used by new tau HLT sequence, but I think that the version in 70x is not using it (otherwise they would complain already). As far as tau reco sequence is concerned, it can be removed, but on the other hand it might be useful in case HLT developers would for some reason try to backport new tau HLT to the 70x release.

@slava77
Copy link
Contributor

slava77 commented Apr 2, 2014

about PFRecoTauDecayModeIndexProducer, yes, it's the same. The point is that 71X is open for new developments and 70X is only for bugfixes (nominally).
PFRecoTauDecayModeIndexProducer is the new feature from 71X that's proposed for 71X.
At the moment, I'm mostly pointing it out, at least for the future reference.

@jpavel
Copy link
Contributor Author

jpavel commented Apr 2, 2014

I see - I was not aware about the practical difference between new developments and patching. I have removed the file as it is not used and if needed it can be resurrected

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2014

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

@slava77
Copy link
Contributor

slava77 commented Apr 2, 2014

additions of unused (in regular workflows) methods should be OK though. I didn't make enough effort to check if this was actually used

@jpavel
Copy link
Contributor Author

jpavel commented Apr 2, 2014

np, I guess we can be without that file if 70x was living happily without it so far. As I said earlier - if requested from HLT side, it will be added.

@jpavel
Copy link
Contributor Author

jpavel commented Apr 2, 2014

Closing as obsolete

@jpavel jpavel closed this Apr 2, 2014
@jpavel jpavel deleted the 70x_taus_backport_71x_Fixes branch May 23, 2014 10:55
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

3 participants