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

reco::FormulaEvaluator segFaults if function string is not hard-coded/pre-defined #21851

Closed
kirschen opened this issue Jan 15, 2018 · 12 comments
Closed

Comments

@kirschen
Copy link
Contributor

Dear experts,

in preparation for a new JEC-release, the parametrization has been extended by a TMath::Exp-expression. However, the the reco::FormulaEvaluator segFaults with this extension.

After inspecting
https://github.com/cms-sw/cmssw/blob/master/CommonTools/Utils/src/FormulaEvaluator.cc#L562-L575
and following, it seems like any new function needs to be announced to the reco::FormulaEvaluator, i.e. if we are to introduce any new function, we should inject it to reco::FormulaEvaluator as early as possible. For now, we replaced "TMath::Exp" by "exp" in our parametrization to make it work.

Would it be possible to catch "undefined" parametrizations instead of segfaulting to make it obvious what goes wrong? Maybe just replace the return in
https://github.com/cms-sw/cmssw/blob/master/CommonTools/Utils/src/FormulaEvaluator.cc#L660
by an error?

Thanks,
Henning for the JERC-group

FYI: Previous "JetMET"-issues with reco::FormulaEvaluator e.g. #16716 (@Dr15Jones @slava77 )

@cmsbuild
Copy link
Contributor

A new Issue was created by @kirschen .

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2018

@kirschen
I think that an error here is reasonable.
Would you like to submit a pull request for this?

@slava77
Copy link
Contributor

slava77 commented Jan 18, 2018

assign analysis,reconstruction

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction,analysis

@slava77,@perrotta,@monttj you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Jan 23, 2018

@kirschen
please clarify if you have a plan to make a PR with the change proposed by you when the issue was opened.
Thank you.

@kirschen
Copy link
Contributor Author

Hi, sorry for the delay. Yes, I can take care of the small PR throwing an exception if none of the predefined functions are found in the next couple of days.

@kirschen
Copy link
Contributor Author

Will link it here, then.

@slava77
Copy link
Contributor

slava77 commented Feb 13, 2018

@kirschen
I wanted to check on the status of that small PR.
Please update.
Thank you.

@kirschen
Copy link
Contributor Author

kirschen commented Mar 2, 2018

Issue is more generically resolved with this PR:
#22354

that gives a useful exception with the initial expression reported here
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' could not parse beyond '[2](31./([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
Copy link
Contributor

perrotta commented Mar 5, 2018

@kirschen : you can close this issue

@kirschen
Copy link
Contributor Author

kirschen commented Mar 5, 2018

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants