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 fixes for Jet toolbox #4021

Merged

Conversation

jstupak
Copy link
Contributor

@jstupak jstupak commented May 27, 2014

Forward port of Dinko's JetDeltaRValueMapProducer.cc, which correctly handles strange corners of phase space.

Changes to RecoJets/JetProducers/BuildFile.xml needed for compilation.

Corrected RecoJets/JetProducers/python/QGTagger_cfi.py to read from condDB rather than a local sqlite file.

Fixed various config files to use correct jet collections.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jstupak for CMSSW_7_1_X.

bug fixes for Jet toolbox

It involves the following packages:

CommonTools/RecoAlgos
RecoJets/JetProducers

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

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

@jstupak what are the expected changes in performance, in particular with AK5->AK4 (comparison results?)? Why is this coming in 71X at this stage and not in 72X first (it's not exactly a bug fix, is it)?

@jstupak
Copy link
Contributor Author

jstupak commented Jun 1, 2014

There is no expected change in performance, nor is this run as part of the standard reconstruction sequence. I would call this a bugfix, since the current code produces an incorrect result.

@StoyanStoynev
Copy link
Contributor

+1
No significant changes observed in RECO.

When merging the PR however "errors" occur which I am told are harmless:

...
error: addinfo_cache failed for path 'CondTools/Hcal/interface/BoostIODBWriter.h'
error: addinfo_cache failed for path 'CondTools/Hcal/interface/BoostIODBReader.h'
Removing RecoPixelVertexing/PixelVertexFinding/python/pixelVertexCollectionTrimmer_cfi.py

...

Comment if I am mistaken.

@StoyanStoynev
Copy link
Contributor

(the large font was unintentional)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2014

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?

@StoyanStoynev
Copy link
Contributor

Actually ignore the additional comment - it is for another pull request (sorry).

@slava77
Copy link
Contributor

slava77 commented Jun 2, 2014

"no significant changes" is not very clear: which variables are affected?
John commented that this code is not run in RECO. If so, and there are changes, do we have a bug?

@StoyanStoynev
Copy link
Contributor

There are no changes in distributions except timing. There are always some variations in timing, here is a non-representative example (from this PR):
example
More timing tests (DQM based) didn't show anything suspicious, and in particular - about jets.
Then, the event size - there are few changes like this (actually I show the largest by far) in the compressed size:
...
Branch Name | Average Uncompressed Size (Bytes/Event) | Average Compressed Size (Bytes/Event)
...

< recoTracks_generalTracks__RECO. 282054 64705.3

recoTracks_generalTracks__RECO. 282054 64693.6

In fact all the minute changes in sizes seem unrelated to the PR. So to recap - nothing points to problems with the PR.

@slava77
Copy link
Contributor

slava77 commented Jun 2, 2014

ok, got it.
what you describe qualifies as "no changes at all" ;)

@jstupak
Copy link
Contributor Author

jstupak commented Jun 3, 2014

So does this mean we are ready for the merge?

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

it will be decided tomorrow at/after the release planning meeting

@jstupak
Copy link
Contributor Author

jstupak commented Jun 3, 2014

okay, thanks!

davidlange6 added a commit that referenced this pull request Jun 3, 2014
@davidlange6 davidlange6 merged commit 860f02b into cms-sw:CMSSW_7_1_X Jun 3, 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

6 participants