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

Use string instead of integer to select WarpX algorithms #168

Merged
merged 13 commits into from
Jun 3, 2019
Merged

Conversation

RemiLehe
Copy link
Member

@RemiLehe RemiLehe commented May 30, 2019

Overview

This PR modifies the way in which users choose the algorithm in the input script: instead of using integers, users now enter a string - which is often more meaningful to them. Note that this will break backward-compatibility ; previous script will abort with an error message explaining that an integer is not a valid algorithm.

Internally, WarpX still uses integers, as a switch for different algorithms. (This is mainly for compatibility with PICSAR, and other Fortran routines.) Therefore, at initialization, the string entered by the user is matched to the appropriate integer using maps. (These maps are not used anymore, after initialization.)

This PR ensures that:

  • The code checks that the user entered a valid string, and aborts the code otherwise, while printing a message that lists the valid strings.
  • If the code was compiled for GPU: options that do not work on GPU can never be selected.
  • A reasonable default is chosen, if the user does not explicitly select an algorithm in their input script.

When reviewing this PR

There are a lot of files changed, because all the input scripts had to be modified. However, when reviewing this PR, I would recommend to look at the following files, in the following order:

  • parameters.rst : documentation explaining the new way in which the user enters the algorithm
  • WarpX.cpp: initialization of the algorithm integers, calling the function GetAlgorithmInteger
  • WarpXAlgorithmSelection.cpp: definition of GetAlgorithmInteger
  • WarpXAlgorithmSelection.H: enumeration defining integers for compatibility with PICSAR/Fortran.

@RemiLehe RemiLehe changed the base branch from master to dev May 30, 2019 03:21
@RemiLehe RemiLehe changed the title [WIP] Use string instead of integer to select WarpX algorithms Use string instead of integer to select WarpX algorithms May 30, 2019
@RemiLehe RemiLehe requested a review from MaxThevenet May 30, 2019 17:28
@RemiLehe RemiLehe added the component: documentation Docs, readme and manual label May 30, 2019
@@ -0,0 +1,88 @@
#include <WarpXAlgorithmSelection.H>

Copy link
Member

Choose a reason for hiding this comment

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

I find that I need to #include <map> here to compile this on Summit.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, thanks. I'll add this.

// Read user input ; use "default" if it is not found
std::string algo = "default";
pp.query( pp_search_key, algo );

Copy link
Member

@atmyers atmyers May 30, 2019

Choose a reason for hiding this comment

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

You could use std::transform(algo.begin(), algo.end(), algo.begin(), ::tolower); here so that users could put either Boris or boris, say.

Source/WarpX.cpp Outdated
ParmParse pp("algo");
current_deposition_algo = GetAlgorithmInteger(pp, "current_deposition");
charge_deposition_algo = GetAlgorithmInteger(pp, "charge_deposition");
field_gathering_algo = GetAlgorithmInteger(pp, "charge_deposition");
Copy link
Member

Choose a reason for hiding this comment

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

Should this be "field_gathering"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes indeed! Thanks for catching this!

@atmyers
Copy link
Member

atmyers commented May 30, 2019

Is it worth distinguishing between things like the CKC field solver, will give the correct answer with USE_GPU=TRUE but will be slow because the routines aren't offloaded, and things like vectorized deposition, which will just crash? Right now the code will abort if CKC is selected and USE_GPU=TRUE.

@RemiLehe
Copy link
Member Author

I think that it's okay to prevent users from using routines that will run on the CPU. (We don't want them to use them by accident without knowing that they will be slow.)

Regarding CKC in particular: we could easily add the required acc directive in picsar, and edit the WarpXAlgorithmSelection.cpp accordingly.

@RemiLehe
Copy link
Member Author

@atmyers Thanks for your comments. I addressed them, and also added the required OpenACC directives for the CKC solver to work on GPU, on the PICSAR side.

In addition, I incorporated the changes from #170, so that the automated tests pass.
So please merge #170 first.

@RemiLehe
Copy link
Member Author

RemiLehe commented Jun 3, 2019

@atmyers I think the PR is ready for merging.

@atmyers atmyers merged commit f2e61f1 into dev Jun 3, 2019
@RemiLehe RemiLehe deleted the algo_selection branch June 14, 2019 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: documentation Docs, readme and manual
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants