add exception for undefined functions to FormulaEvaluator #22216

kirschen · 2018-02-14T13:41:21Z

Follow-up on issue #21851

kirschen · 2018-02-14T13:41:39Z

CommonTools/Utils/src/FormulaEvaluator.cc

@@ -657,7 +664,12 @@ namespace {
      return info;
    }

-    return info;
+    // throw exception if info still undefined and first char doesn't match variable name
+    if( *iBegin == 'x' or *iBegin == 'y' or *iBegin == 'z' or *iBegin == 't' ) {


Would be better to reuse the corresponding function from VariableFinder, but wasn't sure how to do. As checkStart of FunctionFinder checks for std::isalpha(iSymbol), all expressions that start with x/y/z/t are also parsed here.

@Dr15Jones : I agree with @kirschen that it would be preferable to use here the function coded in VariableFinder::checkStart(). Do you have any suggestion about how to do it?

cmsbuild · 2018-02-14T13:41:44Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-02-14T13:45:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22216/3362

cmsbuild · 2018-02-14T13:45:34Z

A new Pull Request was created by @kirschen for master.

It involves the following packages:

CommonTools/Utils

@perrotta, @cmsbuild, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@makortel this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

perrotta · 2018-02-14T13:51:43Z

please test

cmsbuild · 2018-02-14T13:51:59Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26068/console Started: 2018/02/14 17:16

cmsbuild · 2018-02-14T18:07:32Z

-1

Tested at: bda5e01

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

I found follow errors while testing this PR

Failed tests: AddOn

AddOn:

I found errors in the following addon tests:

cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc_l1stage1 --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_25ns : FAILED - time: date Wed Feb 14 18:40:16 2018-date Wed Feb 14 18:35:46 2018 s - exit: 16640

cmsbuild · 2018-02-14T18:07:34Z

Comparison job queued.

slava77 · 2018-02-14T18:42:09Z

the addon failure is in fastsim1 and seems unrelated to this PR

   [0] Processing  Event run: 1 lumi: 1 event: 13 stream: 3
   [1] Running path 'simulation_step'
   [2] Calling method for module FastSimProducer/'fastSimProducer'
Exception Message:
HelixTrajectory: should not be reached (no solution).

@fabiocos @skurz is this something already known and to be fixed soon?

perrotta · 2018-02-14T19:14:00Z

It is unrelated in fact, and #22202, now merged, should fix it. Slava Krutelyov <notifications@github.com> ha scritto:

…

the addon failure is in fastsim1 and seems unrelated to this PR ``` [0] Processing Event run: 1 lumi: 1 event: 13 stream: 3 [1] Running path 'simulation_step' [2] Calling method for module FastSimProducer/'fastSimProducer' Exception Message: HelixTrajectory: should not be reached (no solution). ``` @fabiocos @skurz is this something already known and to be fixed soon? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #22216 (comment)

cmsbuild · 2018-02-14T21:01:30Z

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22216/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017+HARVESTNANOAODMC2017

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 27
DQMHistoTests: Total histograms compared: 2465360
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2465190
DQMHistoTests: Total skipped: 169
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 1.45 KiB( 22 files compared)
Checked 111 log files, 9 edm output root files, 27 DQM output files

fabiocos · 2018-02-15T08:04:29Z

please test

cmsbuild · 2018-02-15T15:52:36Z

The code-checks are being triggered in jenkins.

kirschen · 2018-02-15T15:52:37Z

Just for reference (behavior expected...), an example of an exception thrown in case of unsupported functions (e.g. now after removing TMath::Exp again):

Processing jec_issue_Dec2017.C...
terminate called after throwing an instance of 'cms::Exception'
what(): An exception of category 'FormulaEvaluatorParseError' occurred.
Exception Message:
Undefined function string contained in: 'TMath::Exp(-(TMath::Log(x)-5.030)/0.395)'

cmsbuild · 2018-02-15T15:54:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22216/3389

cmsbuild · 2018-02-15T15:54:26Z

Pull request #22216 was updated. @perrotta, @cmsbuild, @monttj, @slava77 can you please check and sign again.

perrotta · 2018-02-19T10:09:14Z

please test

cmsbuild · 2018-02-19T10:09:31Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26161/console Started: 2018/02/19 11:10

cmsbuild · 2018-02-19T11:42:20Z

+1
Tested at: 3bf55cd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22216/26161/summary.html

cmsbuild · 2018-02-19T11:42:22Z

Comparison job queued.

cmsbuild · 2018-02-19T13:23:55Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 27
DQMHistoTests: Total histograms compared: 2465360
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2465190
DQMHistoTests: Total skipped: 169
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 1.24000000002 KiB( 22 files compared)
Checked 111 log files, 9 edm output root files, 27 DQM output files

perrotta · 2018-02-26T07:15:46Z

@Dr15Jones : while this update could be even accepted as such, as it was pointed out in #22216 (comment) and #22216 (comment) it would be preferable to re-use the corresponding function already coded for the VariableFinder. Since you were the original author of this code, do you have any suggestion about how doing it cleanly? Or would you suggest instead to get rid of it and just stick on what currently coded in this PR?
Thank you.

Dr15Jones · 2018-02-26T15:01:14Z

What was the exact expression that originally failed? I added a simple test evaluating doesNotExist(2) and the code threw an exception. I'm guessing some part of a more complex expression is not properly 'backpropagating' a parsing error up the parsing chain.

kirschen · 2018-02-26T15:08:42Z

@Dr15Jones : The usage of "TMath::Exp" in a formula gave a segfault instead of any exception, as reported in #21851. That triggered this PR. If there is a better way to backpropagate a parsing error than what proposed here, happy to go with that.

Dr15Jones · 2018-02-26T15:15:08Z

@kirschen I added the following unit test

  {
    auto t = [] () {
      reco::FormulaEvaluator f("TMath::Exp(2)");
    };

    CPPUNIT_ASSERT_THROW( t(), cms::Exception );
  }

and that does properly throw an exception. So just calling TMath::Exp does to appear to be sufficient to trigger the segmentation fault. I believe the real error has to do with exactly were in a complex expression an unknown function name is used. Hence why I asked for the original failing full expression.

Dr15Jones · 2018-02-26T15:18:16Z

Just to clarify the intent of the parser, the idea is to allow multiple evaluators to try to parse the expression with a failing to parse be an allowed outcome. Hence, throwing an exception at the point intended could actually break the parser.

Dr15Jones · 2018-02-26T15:41:38Z

After a bit of experimentation, and spotting a place in the code that could cause a segmentation fault, I found the following formula which can cause a crash

reco::FormulaEvaluator f("1 + 2 * 3 + 5 * doesNotExist(2) ");

Dr15Jones · 2018-02-26T15:55:51Z

I believe I have a working fix in #22354 which will handle additional cases that could have lead to a segmentation fault. The ultimate problem was with how the code was handling a failure during the precedence order handling.

Dr15Jones · 2018-02-26T16:00:42Z

@kirschen I appologize for not having looked more deeply at this problem myself in order to realize exactly where the problem lay.

perrotta · 2018-02-27T11:24:38Z

@kirschen , could you please verify if #22354 already fulfills your needs, and you can then close this PR, or if you think that the exception thrown here is still justified to be put in the code together with the fix proposed by @Dr15Jones ?

kirschen · 2018-03-02T13:16:23Z

Hi @Dr15Jones,
no worries. Thanks for providing the much better fix. Happy to close this PR.

#22354 gives a useful exception with the initial troublesome expression:
kirschen@lxplus067 src $ root jec_issue_Dec2017.C
root [0]
Processing jec_issue_Dec2017.C...
terminate called after throwing an instance of 'cms::Exception'
what(): An exception of category 'FormulaEvaluatorParseError' occurred.
Exception Message:
While parsing '[2]([3]([4]+[5]TMath::Log(max([0],min([1],x))))1./([6]+[7]100./3.(TMath::Max(0.,1.03091-0.051154pow(x,-0.154227))-TMath::Max(0.,1.03091-0.051154TMath::Power(208.,-0.154227)))+[8]0.021(-1.+1./(1.+TMath::Exp(-(TMath::Log(x)-5.030)/0.395)))))' could not parse beyond '[2]([3]([4]+[5]TMath::Log(max([0],min([1],x))))1./([6]+[7]100./3.(TMath::Max(0.,1.03091-0.051154pow(x,-0.154227))-TMath::Max(0.,1.03091-0.051154TMath::Power(208.,-0.154227)))+[8]0.021(-1.+1./(1.+'

@perrotta : Thanks for following up on this..

add exception for undefined functions to FormulaEvaluator

bda5e01

cmsbuild added this to the CMSSW_10_1_X milestone Feb 14, 2018

kirschen commented Feb 14, 2018

View reviewed changes

cmsbuild added analysis-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Feb 14, 2018

kirschen mentioned this pull request Feb 14, 2018

add exception for undefined functions to FormulaEvaluator #22212

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 14, 2018

cmsbuild added tests-started and removed tests-pending labels Feb 14, 2018

cmsbuild added tests-rejected and removed tests-started labels Feb 14, 2018

cmsbuild added comparison-available and removed comparison-pending labels Feb 14, 2018

cmsbuild added the tests-pending label Feb 15, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 15, 2018

cmsbuild added tests-started and removed tests-pending labels Feb 19, 2018

cmsbuild added tests-approved and removed tests-started labels Feb 19, 2018

cmsbuild added comparison-available and removed comparison-pending labels Feb 19, 2018

kirschen closed this Mar 2, 2018

kirschen deleted the master_FormulaEvaluatorExceptionAddition branch March 2, 2018 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add exception for undefined functions to FormulaEvaluator #22216

add exception for undefined functions to FormulaEvaluator #22216

kirschen commented Feb 14, 2018

kirschen Feb 14, 2018

perrotta Feb 19, 2018

cmsbuild commented Feb 14, 2018

cmsbuild commented Feb 14, 2018

cmsbuild commented Feb 14, 2018

perrotta commented Feb 14, 2018

cmsbuild commented Feb 14, 2018 •

edited

cmsbuild commented Feb 14, 2018

cmsbuild commented Feb 14, 2018

slava77 commented Feb 14, 2018

perrotta commented Feb 14, 2018 via email

cmsbuild commented Feb 14, 2018

fabiocos commented Feb 15, 2018

cmsbuild commented Feb 15, 2018

kirschen commented Feb 15, 2018

cmsbuild commented Feb 15, 2018

cmsbuild commented Feb 15, 2018

perrotta commented Feb 19, 2018

cmsbuild commented Feb 19, 2018 •

edited

cmsbuild commented Feb 19, 2018

cmsbuild commented Feb 19, 2018

cmsbuild commented Feb 19, 2018

perrotta commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

kirschen commented Feb 26, 2018 •

edited

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

perrotta commented Feb 27, 2018

kirschen commented Mar 2, 2018

add exception for undefined functions to FormulaEvaluator #22216

add exception for undefined functions to FormulaEvaluator #22216

Conversation

kirschen commented Feb 14, 2018

kirschen Feb 14, 2018

Choose a reason for hiding this comment

perrotta Feb 19, 2018

Choose a reason for hiding this comment

cmsbuild commented Feb 14, 2018

cmsbuild commented Feb 14, 2018

cmsbuild commented Feb 14, 2018

perrotta commented Feb 14, 2018

cmsbuild commented Feb 14, 2018 • edited

cmsbuild commented Feb 14, 2018

cmsbuild commented Feb 14, 2018

slava77 commented Feb 14, 2018

perrotta commented Feb 14, 2018 via email

cmsbuild commented Feb 14, 2018

fabiocos commented Feb 15, 2018

cmsbuild commented Feb 15, 2018

kirschen commented Feb 15, 2018

cmsbuild commented Feb 15, 2018

cmsbuild commented Feb 15, 2018

perrotta commented Feb 19, 2018

cmsbuild commented Feb 19, 2018 • edited

cmsbuild commented Feb 19, 2018

cmsbuild commented Feb 19, 2018

cmsbuild commented Feb 19, 2018

perrotta commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

kirschen commented Feb 26, 2018 • edited

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

Dr15Jones commented Feb 26, 2018

perrotta commented Feb 27, 2018

kirschen commented Mar 2, 2018

cmsbuild commented Feb 14, 2018 •

edited

cmsbuild commented Feb 19, 2018 •

edited

kirschen commented Feb 26, 2018 •

edited