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

support mathematical expressions in Parameter #2325

Merged
merged 8 commits into from
Jan 31, 2019

Conversation

norihiro-w
Copy link
Collaborator

@norihiro-w norihiro-w commented Jan 17, 2019

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

  • Currently "x", "y", "z" are supported as variables.
  • Example of input files is
        <parameter>
            <name>initial_pressure</name>
            <type>Function</type>
            <expression>1000.0*9.81*(-z)</expression>
        </parameter>
  • For non-scalar parameters, <expression> definition should be given for each component. The number of parameter components is set by the number of given <expression>.

@endJunction
Copy link
Member

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.
Maybe "SpatialFunction" would be more precise. With possible variations on "TemporalFunction" or "SpatioTemporalFunction". More suggestions are welcome.

mutable double _x, _y, _z;
symbol_table_t _symbol_table;
std::vector<expression_t> _vec_expression;
mutable std::vector<T> _cache;
Copy link
Member

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().

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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(),
Copy link
Member

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.

Copy link
Collaborator Author

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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@wenqing wenqing left a 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>
Copy link
Member

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.

Copy link
Collaborator Author

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maethmatical --> mathematical

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variales -> variables

Copy link
Collaborator Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functons -> functions

Copy link
Collaborator Author

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;
Copy link
Member

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?

Copy link
Collaborator Author

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"))
Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@TomFischer TomFischer left a 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: ⏩

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #2325 into master will increase coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
GeoLib/SurfaceGrid.cpp 53.7% <0%> (-2.78%) ⬇️
GeoLib/Surface.cpp 56.81% <0%> (-2.28%) ⬇️
GeoLib/Polyline.cpp 35.75% <0%> (-0.61%) ⬇️
...ousMedium/Permeability/createPermeabilityModel.cpp 0% <0%> (ø) ⬆️
Applications/ApplicationsLib/ProjectData.cpp 0% <0%> (ø) ⬆️
...Lib/PorousMedium/Permeability/DupuitPermeability.h

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b910d25...1c2a783. Read the comment docs.

@endJunction endJunction merged commit 7e0dc96 into ufz:master Jan 31, 2019
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

@norihiro-w norihiro-w deleted the add-exprtk branch September 26, 2022 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants