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

Add Parser #595

Merged
merged 20 commits into from
Sep 23, 2021
Merged

Conversation

AlexanderSinn
Copy link
Member

This PR is based on #594

The use of amrex::Parser allows for expressions to be parsed from the input file instead of just numbers. In this PR all input Parameters, even the ones queried by AMReX, are obtained with this Parser, except for string.
It is possible to define user constants in the input script with my_constants. Some Physical constants are already provided.

my_constants.kp_inv = 10e-6
my_constants.kp = 1 / kp_inv
my_constants.wp = clight * kp
my_constants.ne = wp^2 * m_e * epsilon0 / q_e^2
plasma.density = ne

Already implemented but currently unused is the ability to define functions in the input script which can be evaluated in the GPU.

  • Small enough (< few 100s of lines), otherwise it should probably be split into smaller PRs
  • Tested (describe the tests in the PR description)
  • Runs on GPU (basic: the code compiles and run well with the new module)
  • Contains an automated test (checksum and/or comparison with theory)
  • Documented: all elements (classes and their members, functions, namespaces, etc.) are documented
  • Constified (All that can be const is const)
  • Code is clean (no unwanted comments, )
  • Style and code conventions are respected at the bottom of https://github.com/Hi-PACE/hipace
  • Proper label and GitHub project, if applicable

@AlexanderSinn AlexanderSinn requested review from SeverinDiederichs and MaxThevenet and removed request for SeverinDiederichs September 16, 2021 01:07
Copy link
Member

@MaxThevenet MaxThevenet left a comment

Choose a reason for hiding this comment

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

Excellent, thanks a lot for this great PR!! You will find a few minor comments below.
As you mentioned in the description, the capability to read a function is implemented in getFunctionWithParser, but not used. I think the most useful one would be for <plasma>.density. Do you think you could implement that in a separate PR? I would suggest to read e.g.

<plasma>.density(x,y,z) = ne*(1.+(x^2+y*2)/R^2)

where we explicitly add (x,y,z) to the name of the queried parameter, just to inform the user that a function of space can be read here, and that the coordinates are called x, y and z.

src/Hipace.cpp Show resolved Hide resolved
Comment on lines 31 to 32
{"true", 1.},
{"false", 0.}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{"true", 1.},
{"false", 0.}
{"true", 1},
{"false", 0}

// in case an infinite recursion is found (a symbol's value depending on itself).
static std::set<std::string> recursive_symbols{};

//amrex::Parser parser(parse_function);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//amrex::Parser parser(parse_function);

Comment on lines 133 to 134
static amrex::Parser&
initParser (amrex::Parser& parser, amrex::Vector<std::string> const& varnames) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to return the parser instead of only modifying the input argument? Could you do

void
initParser (amrex::Parser& parser, amrex::Vector<std::string> const& varnames) {

and call

initParser(parser, {}).compileHost<0>()();

instead of

val = initParser(parser, {}).compileHost<0>()();

?

Comment on lines 7 to 10
my_constants.kp_inv = 10e-6
my_constants.kp = 1 / kp_inv
my_constants.wp = clight * kp
my_constants.ne = wp^2 * m_e * epsilon0 / q_e^2
Copy link
Member

Choose a reason for hiding this comment

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

Would that work too? Just to remind users that an int and a float are written differently. This also applies below.

Suggested change
my_constants.kp_inv = 10e-6
my_constants.kp = 1 / kp_inv
my_constants.wp = clight * kp
my_constants.ne = wp^2 * m_e * epsilon0 / q_e^2
my_constants.kp_inv = 10.e-6
my_constants.kp = 1. / kp_inv
my_constants.wp = clight * kp
my_constants.ne = wp^2 * m_e * epsilon0 / q_e^2

Also, while this is exactly how we convert between the SI and normalised input files, this is not what a new user would do (typically, they would set the electron density ne, and calculate kp etc. from it). Could you check this with @SeverinDiederichs?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but no dot also works because the parser always uses double internally.

blowout_wake/inputs_ionization_SI calculates kp_inv from ne. Should another file do this too?

Copy link
Member

Choose a reason for hiding this comment

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

Let us keep this discussion for later. I think that examples/blowout_wake/inputs_SI is the first example people would look at, because it is the most usual use case, so we should make it meaningful. But let's keep it as is for now.

Comment on lines 25 to 26
geometry.prob_lo = -8*kp_inv -8*kp_inv -6*kp_inv # physical domain
geometry.prob_hi = 8*kp_inv 8*kp_inv 6*kp_inv
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
geometry.prob_lo = -8*kp_inv -8*kp_inv -6*kp_inv # physical domain
geometry.prob_hi = 8*kp_inv 8*kp_inv 6*kp_inv
geometry.prob_lo = -8.*kp_inv -8.*kp_inv -6.*kp_inv # physical domain
geometry.prob_hi = 8.*kp_inv 8.*kp_inv 6.*kp_inv


beams.names = beam
beam.injection_type = fixed_ppc
beam.profile = gaussian
beam.zmin = -59.e-6
beam.zmax = 59.e-6
beam.radius = 12.e-6
beam.density = 8.47187610257747e23 # 3*ne
beam.density = 3*ne
Copy link
Member

Choose a reason for hiding this comment

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

Lovely. Do you know when double-quotes "3*ne" are required? I assume it is at least the case for multi-line expressions, but apart from that does this always work without double quotes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Quotes are needed around the individual expressions of an array type like geometry.prob_lo if the expressions contain spaces. I also found that quotes can help if the expression uses functions like exp(x) and is inside a bash script.

@MaxThevenet MaxThevenet merged commit 2852f7a into Hi-PACE:development Sep 23, 2021
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.

2 participants