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

Matheval #1644

Closed
wants to merge 60 commits into from
Closed

Matheval #1644

wants to merge 60 commits into from

Conversation

hmenke
Copy link
Member

@hmenke hmenke commented Nov 23, 2017

What?

This adds a mathematical expression parser to the ESPResSo core. With this it is possible to pass a mathematical expression as a string all the way into a tight loop, like the integration loop and have it evaluated in every iteration with, say, the time step.

Why?

This is intended to serve as a building block for constraints or LB boundaries with time-dependent parameters. It was motivated by the Lees-Edwards boundary conditions for LB as the Lees-Edwards shift has to be adjusted every time step.

How?

The parser uses Boost.Spirit X3 and Boost.Fusion under the hood. The compilation time of projects using Boost.Spirit is unfortunately quite excessive, which is why I hide all those nasty details behind an opaque pointer (also known as pImpl).


A unit test and inline Doxygen documentation is supplied alongside.

  • Tests
  • Docs

Right now some of the checks are failing because the Boost version in some of the containers is too old. The oldest Boost version with which it seems to work fine is 1.65.1 (maybe it works with an even older version, but that is the oldest version in the Docker containers for which it works).

@codecov
Copy link

codecov bot commented Nov 23, 2017

Codecov Report

❗ No coverage uploaded for pull request base (python@4256fd1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             python   #1644   +/-   ##
========================================
  Coverage          ?     85%           
========================================
  Files             ?     520           
  Lines             ?   24496           
  Branches          ?       0           
========================================
  Hits              ?   20994           
  Misses            ?    3502           
  Partials          ?       0

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 4256fd1...fb6df24. Read the comment docs.

@KaiSzuttor
Copy link
Member

Do we want to depend on a self maintained lib?

@mkuron
Copy link
Member

mkuron commented Nov 23, 2017

Considering it consists of just one header file and isn't likely to have a high update frequency we might as well copy that into Espresso.

@hmenke hmenke force-pushed the matheval branch 2 times, most recently from d035ea9 to 1ca0b8b Compare November 28, 2017 06:20
@KaiSzuttor
Copy link
Member

Unknown feature 'EXPRESSION'

@RudolfWeeber
Copy link
Contributor

I guarded all uses of the matheval library in Espresso, so it should build without it, in case the need arises.

I used git subtree to integrate the library, in the end, because unless we upgrade to cmake 3.11 and fetch_content, some changes to mathevalś cmake were needed.

From my side, we can go ahead with this pr. However, if someone could push better cmake in the root CMakeList.txt to detect the presence of the library, that would be helpful.

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

I cannot compile this PR on maxset:

cp ../maintainer/configs/maxset.hpp myconfig.hpp
cmake ..
make
External feature 'MATHEVAL' can not be defined in myconfig.
There were errors in '/work/jgrad/espresso-fork/build/src/config/myconfig-final.hpp'

testsuite/python/generic.py Outdated Show resolved Hide resolved
@fweik
Copy link
Contributor

fweik commented Feb 4, 2020

@jngrad you potentially need #3446 for this to work.

@KaiSzuttor
Copy link
Member

ping

@KaiSzuttor
Copy link
Member

pong

@RudolfWeeber
Copy link
Contributor

After a renewed discussion in the core team, we have come to the conclusion that we are not able to maintain the code introduced by this PR.
Therefore, we are, unfortunately, not going to merge this PR.

I'd like to emphasize that this is not a matter of code quality.
However, merging this would imply keeping some 800 lines of math parsing code and a dependency on the boost spirit library working for the forseeable future.
Given the rate at which OS, library, and compiler versions evolve, this is a promise we don't feel we can make for code which does not directly relate to Espresso's core purpose.

The functionality provided by this Pr is of interest but a very similar result can be obtained using tabulated potentials.

@hmenke
Copy link
Member Author

hmenke commented Aug 3, 2020

we are not able to maintain the code introduced by this PR

Of course not.

Have a good day.

@hmenke hmenke closed this Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants