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

72x taus sqlite hook #4686

Merged
merged 4 commits into from
Jul 25, 2014
Merged

Conversation

jpavel
Copy link
Contributor

@jpavel jpavel commented Jul 16, 2014

The hooks for the sqlite file reading that have been originally in #4585. There have been reported differences in anti-electron config, that have been caused by typo in config files and fixed. The difference in isolation discriminators was not observed, so perhaps it have been fixed already.

This pull request does not change the RECO sequence at all, but adds new function to switch MVA input to a DB file

EDIT: to test new feature, simply add following lines to your config file

from RecoTauTag.Configuration.switchMVAtoDB_cfi import switchMVAtoDB
process = switchMVAtoDB(process) # due to the input file location it works only on lxplus

@cmsbuild
Copy link
Contributor

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

72x taus sqlite hook

It involves the following packages:

RecoTauTag/Configuration
RecoTauTag/RecoTau

@nclopezo, @cmsbuild, @Degano, @StoyanStoynev, @slava77 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.

@cmsbuild
Copy link
Contributor

-1
Tested at: b6f482d
I found an error when building:

>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/AMPTInterface/src/art1f.f 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/AMPTInterface/src/hijing1.383_ampt.f 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/AMPTInterface/src/hipyset1.35.f 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/AMPTInterface/src/linana.f 
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/RivetInterface/plugins/RivetHarvesting.cc: In constructor 'RivetHarvesting::RivetHarvesting(const edm::ParameterSet&)':
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/RivetInterface/plugins/RivetHarvesting.cc:48:30: error: 'CmpAnaHandle' was not declared in this scope
   const std::set< AnaHandle, CmpAnaHandle > & analyses = _analysisHandler.analyses();
                              ^
/build/cmsbuild/jenkins-workarea/workspace/ib-integration-CMSSW_7_2_X-slc6_amd64_gcc481/CMSSW_7_2_X_2014-07-16-0200/src/GeneratorInterface/RivetInterface/plugins/RivetHarvesting.cc:48:43: error: template argument 2 is invalid
   const std::set< AnaHandle, CmpAnaHandle > & analyses = _analysisHandler.analyses();
                                           ^


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4686/358/summary.html

@StoyanStoynev
Copy link
Contributor

@cmsbuild Let's get jenkins rerun. Although the IB used, CMSSW_7_2_X_2014-07-16-0200, is listed to have no build errors (has other errors/warnings) I encounter the error above locally as well. The error has nothing to do with this PR. The PR complies fine with a more recent IB - I checked CMSSW_7_2_X_2014-07-17-0600. Please rerun tests.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

Checking... The PR is identical to (part of) #4585 that was causing some unexplained performance changes initially (later just dropped the problematic part).

@StoyanStoynev
Copy link
Contributor

+1
Tested b6f482d on top of CMSSW_7_2_X_2014-07-18-1400. Short and extended matrix tests fine. No diffs with fwlite scripts and I checked in particular wf 202 and 38 with DQM based scripts - no diff. None expected. In addition wf 1000 has no diffs locally though jenkins gave some suspicious output (pages 313-330; the rest are known false positives...): https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_2_X_2014-07-18-0200+4686/3154/alternative-comparisons/1000.0-result.ps.gz
I didn't find anything to comment on the code. And I didn't test if the newly added functionality actually works (to comment below).

First I was testing with an older IB, CMSSW_7_2_X_2014-07-17-0600, and the jobs (for the IB, not even the IB+PR) were running forever. I did not investigate given the newer IB was fine.

@StoyanStoynev
Copy link
Contributor

@jpavel how do actually check the new functionality works properly? Can you add (in another PR) some testing configuration file in test/ ?

@StoyanStoynev
Copy link
Contributor

Fine, there should be a better way but I guess I would have to dig it up if suggesting it, will see.
@jpavel you missed to follow up on the solution above for PFRecoTauDiscriminationAgainstMuonMVA.cc, didn't you? Which prompts me to also note there is code repetition - maybe think about some code optimization in a following PR.

@jpavel
Copy link
Contributor Author

jpavel commented Jul 23, 2014

@StoyanStoynev - yes I missed that one (and also similar case for antiElectron). You are right about the code repetition, we will try to solve it in the future. Also, I updated the PR description with the recipe of how to test the new feature.

@cmsbuild
Copy link
Contributor

Pull request #4686 was updated. @nclopezo, @cmsbuild, @Degano, @StoyanStoynev, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@StoyanStoynev
Copy link
Contributor

+1
There are no differences on plots (tested again as above this time for f787ad8 on top of CMSSW_7_2_X_2014-07-21-0200 ), jenkins are all fine. The (few thousand lines of) code looks fine to me but I still want to test the memory flow with and without the new option provided. Will not happen soon (I am kind of away at the moment) but will try to follow up. In any case the default behavior of the code seems (visually) preserved.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

nclopezo added a commit that referenced this pull request Jul 25, 2014
RecoTauTag -- 72x taus sqlite hook
@nclopezo nclopezo merged commit b921146 into cms-sw:CMSSW_7_2_X Jul 25, 2014
@jpavel jpavel deleted the 72x_taus_SQliteHook branch August 6, 2014 13:55
jpavel pushed a commit to jpavel/cmssw that referenced this pull request Aug 21, 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