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

Projects
None yet
4 participants
@norihiro-w
Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member

endJunction commented Jan 17, 2019

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;

This comment has been minimized.

@endJunction

endJunction Jan 17, 2019

Member

The mutable members are not compatible with openMP parallelization. Better to use local variables in the operator().

This comment has been minimized.

@norihiro-w

norihiro-w Jan 21, 2019

Author Collaborator

@endJunction I will change it. FYI other parameters also have mutable members.

This comment has been minimized.

@norihiro-w

norihiro-w Jan 30, 2019

Author Collaborator

@endJunction no it's no easy. Because the operator() returns a reference, local variables cannot be used here.

This comment has been minimized.

@norihiro-w

norihiro-w Jan 30, 2019

Author Collaborator

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

This comment has been minimized.

@endJunction

endJunction Jan 17, 2019

Member

Replace the ERR with OGS_FATAL. Somebody is going to miss that error message otherwise.

This comment has been minimized.

@norihiro-w

norihiro-w Jan 30, 2019

Author Collaborator

done


std::vector<std::string> vec_expressions;

//! \ogs_file_param{prj__parameters__parameter__Function__expression}

This comment has been minimized.

@endJunction

This comment has been minimized.

@norihiro-w

norihiro-w Jan 30, 2019

Author Collaborator

done

@wenqing
Copy link
Member

wenqing left a comment

👍

#include <utility>
#include <vector>

#include <exprtk.hpp>

This comment has been minimized.

@wenqing

wenqing Jan 18, 2019

Member

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.

This comment has been minimized.

@norihiro-w

norihiro-w Jan 21, 2019

Author Collaborator

i tested with VS2017 and so far it's ok.

{

/// A parameter class evaluating a functon defined by
/// a user-provided maethmatical expression.

This comment has been minimized.

@wenqing

wenqing Jan 18, 2019

Member

maethmatical --> mathematical

This comment has been minimized.

@norihiro-w

norihiro-w Jan 30, 2019

Author Collaborator

done

@norihiro-w norihiro-w force-pushed the norihiro-w:add-exprtk branch 2 times, most recently from 5bce310 to bff354a Jan 30, 2019

@@ -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.

This comment has been minimized.

@TomFischer

TomFischer Jan 31, 2019

Member

variales -> variables

This comment has been minimized.

@norihiro-w

norihiro-w Jan 31, 2019

Author Collaborator

fixed

namespace ProcessLib
{

/// A parameter class evaluating functons defined by

This comment has been minimized.

@TomFischer

TomFischer Jan 31, 2019

Member

functons -> functions

This comment has been minimized.

@norihiro-w

norihiro-w Jan 31, 2019

Author Collaborator

fixed


private:
MeshLib::Mesh const& _mesh;
std::vector<std::string> _vec_expression_str;

This comment has been minimized.

@TomFischer

TomFischer Jan 31, 2019

Member

Can _vec_expression_str be const?

This comment has been minimized.

@norihiro-w

norihiro-w Jan 31, 2019

Author Collaborator

done

std::vector<std::string> vec_expressions;

//! \ogs_file_param{prj__parameters__parameter__Function__expression}
for (auto p : config.getConfigSubtreeList("expression"))

This comment has been minimized.

@TomFischer

TomFischer Jan 31, 2019

Member

for (auto const& p : config.getConfigSubtreeList("expression")) avoid copies.

This comment has been minimized.

@norihiro-w

norihiro-w Jan 31, 2019

Author Collaborator

fixed

@TomFischer
Copy link
Member

TomFischer left a comment

After fixing / discussion of open points:

@norihiro-w norihiro-w force-pushed the norihiro-w:add-exprtk branch from 0946bc0 to e9f3ad5 Jan 31, 2019

@codecov

This comment has been minimized.

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

5 checks passed

codecov/patch Coverage not affected when comparing b910d25...1c2a783
Details
codecov/project 32.9% (+0.45%) compared to b910d25
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.