-
Notifications
You must be signed in to change notification settings - Fork 11
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
update logic of REST_StringHelper::isAExpression() #188
Conversation
It is better to wait for pipeline is green to ask for review. There is a risk the review will be forgotten, because if it is running or red I will change to do something else, and I do not have a new notificiation to remind me I have to review this PR. |
Which is the nature of the error at the main framework pipeline? @lobis In |
Should not be simply used |
We could perhaps implement |
What do you mean? I see it on |
/// example: | ||
/// 1+1 : expression | ||
/// sin(1.5) : expression | ||
/// 123456789 : not expression, It is a pure number that can be directly parsed. | ||
Int_t REST_StringHelper::isAExpression(string in) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REST_StringHelper::isAExpression(const string& in)
Ok, it is there at master. Probably I was looking at a branch. |
return 0; | ||
|
||
vector<string> funcs{"sqrt", "log", "exp", "gaus", "cos", "sin", "tan", "atan", "acos", "asin"}; | ||
for (int i = 0; i < funcs.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (const auto& item: funcs){
if (in.find(item) != std::string::npos) {
symbol = true;
break;
}
}
But my question to you was "Which is the nature of the error at the main framework pipeline? How do we debug that?" @lobis |
It looks very similar to what I was encountering due to The main cause of the problem is still not understood by me and the dirty fix was the ignore some code on My bet for this failure is that maybe some of the changes I made to avoid But the small changes in this PR doesn't suggest how this could end up affecting unit testing... |
But it says |
Today this has happened a few times, maybe there is an issue with the gitlab or our network |
I am still not sure why pipeline is failing, but in this PR #189 something similar seems to be taking place... Locally everything works but in the pipeline fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be simply used Int_t REST_StringHelper::isANumber(string in) inside the implementation? Perhaps a new method isAnInteger
would be handy?
…ard c++ type name(long long)
Previously pure number will also be regarded as an expression. In this case long numbers are processed by TFormula and converted into scientific notation, and loses precision.
For example, in the old logic, the parameter:
will be processed by
REST_StringHelper::EvaluateExpression()
. It will be converted firstly into (double)1.2345789e+08, and then converted into (string)"1.23457e+08", and then back to (int)123457000. This is problematic for seed definition.So this PR updates the logic of expression identification, inside
REST_StringHelper::isAExpression()
. The simulation results may be different.restG4 pipeline for branch
nkx111-patch-3
: