-
Notifications
You must be signed in to change notification settings - Fork 239
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
support mathematical expressions in Parameter #2325
Conversation
I'd add a git submodule instead. Same source code by same author here: https://github.com/ArashPartow/exprtk "Function" is OK. If we add python functions later, they can be called "Python(Function)"; no clash here. |
mutable double _x, _y, _z; | ||
symbol_table_t _symbol_table; | ||
std::vector<expression_t> _vec_expression; | ||
mutable std::vector<T> _cache; |
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.
The mutable members are not compatible with openMP parallelization. Better to use local variables in the operator()
.
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.
@endJunction I will change it. FYI other parameters also have mutable members.
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.
@endJunction no it's no easy. Because the operator()
returns a reference, local variables cannot be used here.
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.
i have created #2337 for this.
parser_t parser; | ||
if (!parser.compile(_vec_expression_str[i], _vec_expression[i])) | ||
{ | ||
ERR("Error: %s\tExpression: %s\n", parser.error().c_str(), |
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.
Replace the ERR with OGS_FATAL. Somebody is going to miss that error message otherwise.
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.
done
|
||
std::vector<std::string> vec_expressions; | ||
|
||
//! \ogs_file_param{prj__parameters__parameter__Function__expression} |
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.
the tag documentation files are missing. See https://github.com/ufz/ogs/tree/master/Documentation/ProjectFile/prj/parameters/parameter
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.
done
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.
👍
#include <utility> | ||
#include <vector> | ||
|
||
#include <exprtk.hpp> |
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.
The file is large with template definitions. When it was compiled in OGS5 with visual studio 2010, the option of /bigobj is needed. Since the building in appveyor succeeded, I think the new visual studio can handle it without using any additional option.
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.
i tested with VS2017 and so far it's ok.
{ | ||
|
||
/// A parameter class evaluating a functon defined by | ||
/// a user-provided maethmatical expression. |
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.
maethmatical --> mathematical
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.
done
5bce310
to
bff354a
Compare
@@ -0,0 +1 @@ | |||
Mathematical expression of the function (currently x,y,z are supported variales). For non-scalar values, an expression should be given for each component. |
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.
variales -> variables
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.
fixed
namespace ProcessLib | ||
{ | ||
|
||
/// A parameter class evaluating functons defined by |
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.
functons -> functions
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.
fixed
|
||
private: | ||
MeshLib::Mesh const& _mesh; | ||
std::vector<std::string> _vec_expression_str; |
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.
Can _vec_expression_str
be const
?
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.
done
std::vector<std::string> vec_expressions; | ||
|
||
//! \ogs_file_param{prj__parameters__parameter__Function__expression} | ||
for (auto p : config.getConfigSubtreeList("expression")) |
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 (auto const& p : config.getConfigSubtreeList("expression"))
avoid copies.
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.
fixed
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.
After fixing / discussion of open points: ⏩
0946bc0
to
e9f3ad5
Compare
Codecov Report
@@ Coverage Diff @@
## master #2325 +/- ##
=========================================
+ Coverage 32.45% 32.9% +0.45%
=========================================
Files 526 525 -1
Lines 20008 19712 -296
Branches 4051 4050 -1
=========================================
- Hits 6493 6487 -6
+ Misses 10272 9981 -291
- Partials 3243 3244 +1
Continue to review full report at Codecov.
|
OpenGeoSys development has been moved to GitLab. |
as titled. The implementation is based on exprtk (http://www.partow.net/programming/exprtk/index.html).
So far I named the new parameter type as "Function", but you might have better idea about the naming. let me know...
Remarks
<expression>
definition should be given for each component. The number of parameter components is set by the number of given<expression>
.