Fix precedence error and added additional functions

[Dr15Jones]

Further tests of expressions from the data base showed that there
were additional ones that couldn't be handled.

[cmsbuild]

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_6_X.

Fix precedence error and added additional functions

It involves the following packages:

CommonTools/Utils

@cmsbuild, @cvuosalo, @vadler, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@makortel 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[Dr15Jones]

please test

[cmsbuild]

The tests are being triggered in jenkins.
JENKINS_TEST_URL

[cmsbuild]

+1
Tested at: 99529d8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11684/8682/summary.html

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11684/8682/summary.html

[slava77]

+1
post-merge

for 99529d8

• no changes in jenkins tests and all addOns pass now

[Dr15Jones]

@slava77 thanks for running additional tests. Are the addOnTests only run under certain circumstances? I just wonder since the original pull request didn't seem to run those tests.

[slava77]

Chris, I didn't run them specially for this, addOns are now a part of PR testing
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11684/8682/summary.html

[Dr15Jones]

So I guess the question is why didn't the problem occur during the testing of the original pull request. Were the addOnTests recently changed which caused a different formula to be used? I'm mostly just curious and since the problem seems over now it isn't worth anyones time investigating the matter.

[slava77]

On 10/8/15 8:19 AM, Chris Jones wrote:

So I guess the question is why didn't the problem occur during the
testing of the original pull request. Were the addOnTests recently
changed which caused a different formula to be used? I'm mostly just
curious and since the problem seems over now it isn't worth anyones time
investigating the matter.

I'm guessing the new GT with new jet correction formulas arrived after
the pull request was tested

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

[diguida]

@Dr15Jones @slava77
Thanks for your checks.
Let me make a bit of history:

1. the replacement for the TFormula usage in Replace TFormula in SimpleJetCorrector #11584 was tested over the same IB as New GTs for 76X: L1,HLT,Geometry and HCAL changes #11609 https://cmssdt.cern.ch/SDT/html/showIB.html#CMSSW_7_6_X_2015-09-30-2300 where the GT was still containing the old JEC, and entered https://cmssdt.cern.ch/SDT/html/showIB.html#CMSSW_7_6_X_2015-10-05-2300
2. the GT with the new JEC entered in New GTs for 76X: L1,HLT,Geometry and HCAL changes #11609 was tested over https://cmssdt.cern.ch/SDT/html/showIB.html#CMSSW_7_6_X_2015-09-30-2300 and was merged into IB https://cmssdt.cern.ch/SDT/html/showIB.html#CMSSW_7_6_X_2015-10-06-2300
3. the issues with addOn tests are indeed visible since IB https://cmssdt.cern.ch/SDT/html/showIB.html#CMSSW_7_6_X_2015-10-06-2300 as you can see in https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc493/CMSSW_7_6_X_2015-10-06-2300/addOnTests/logs/cmsDriver-hlt_data_50nsGRun_cmsDriver.py_RelVal_-s_HLT:50nsGRun,RAW2DIGI,L1Reco,RECO_--data_--scenario=pp_-n_10_--conditions_auto:run2_data_50nsGRun_--relval_9000,50_--datatier_.log

The issue was therefore due to the concurrent change of TFormula in a way that was not covered by unit tests: i.e. a regression due to data mutation not covered by tests.

As promised, I can provide you privately the list of all formulas stored in DB, in case you want to expand the unit tests.

I have also a proposal: I will inform JetMet contacts to produce JEC formulas without any TMath expression. Is this ok?

@mmusich something to remember :-)

[Dr15Jones]

@diguida So it is the case of two pull requests come at the same time who work great independently but together interfere. It was a known low probability occurance than.

As for TMath there is no real need to avoid that. The present formula evaluator only knows about a small subset of functions which were the ones I could easily find on my own when running the RelVals. It is normally easy to add a new function to the list. The hold up this time was I uncovered an unrelated bug. Therefore a list of all used formulas would be very helpful since I could be sure that all the functions they used are in the internal list of known functions.