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

[Feature Contribution] Scafacos #989

Merged
merged 64 commits into from Sep 5, 2018

Conversation

@rhalver
Copy link
Collaborator

rhalver commented Jul 9, 2018

Purpose

Fix for the ScaFaCoS library[1], a library for the computation of electro-statics, providing several solvers to choose from, e.g. FMM or P3M. The library was developed in a project funded by the
German Ministry of Science and Education from 2009-2012.

Author(s)

Rene Halver, Juelich Supercomputing Centre, Forschungszentrum Juelich, Germany

Backward Compatibility

No changes in backward compatibility are expected.

Implementation Notes

No other features are influenced by this fix. Within the fix the Coulomb field and potentials
are computed and resulting from them the forces and energy are calculated.Some
changes might be required to completely adhere to the LAMMPS environment.

Post Submission Checklist

Please check the fields below as they are completed

  • The feature or features in this pull request is complete
  • Suitable new documentation files and/or updates to the existing docs are included
  • One or more example input decks are included
  • The source code follows the LAMMPS formatting guidelines

Further Information, Files, and Links

[1] www.scafacos.de

@rhalver rhalver requested review from junghans and rbberger as code owners Jul 9, 2018

@akohlmey akohlmey removed their assignment Jul 10, 2018

@akohlmey akohlmey requested a review from sjplimp Jul 10, 2018

#define LMP_FIX_SCAFACOS_H

#include "fix.h"
#include "fcs.h"

This comment has been minimized.

@akohlmey

akohlmey Jul 11, 2018

Member

please try to avoid non-LAMMPS and non-standard include statements in the header file. all style specific headers need to be included in some classes and name clashes can happen easily (and have happened). instead, you should try to use a forward declaration. e.g. like adding the following inside the fix class:

struct fcsdata *fcs;

and then define the fcsdata struct inside the .cpp file with all the specific data types and information as struct members and allocate (free) it in the constructor (destructor). this way all scafacos specific types and information only have to show up inside the .cpp file and are opaque elsewhere. therefore clashes are no longer possible.

@sjplimp sjplimp self-assigned this Jul 11, 2018

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 11, 2018

For convenience, can the scafacos library be bundled with LAMMPS in /lib, or is there a reason why not, like it is too large or the licenses are incompatible?

#endif
#endif

/* ERROR/WARNING messages:

This comment has been minimized.

@akohlmey

akohlmey Jul 11, 2018

Member

This part of the header lists errors and warnings from fix latte. This should instead list the corresponding errors and warnings in the fix scafacos code and the references to fix latte removed.

{
if (result)
{
printf("ScaFaCoS Error: Caught error on task %d.\n", comm_rank);

This comment has been minimized.

@akohlmey

akohlmey Jul 11, 2018

Member

Please don't use printf(), but fprintf(screen,...) and fprintf(logfile, ...) provided either FILE pointer is non-NULL. Please note, that calling error->all() requires, that this function is called from all MPI ranks on the communicator world, otherwise you need to call error->one(). If however check_results() is called from all MPI ranks on world, then only MPI rank 0 should be producing output from fprintf() or else the screen would have all messages replicated by the number of MPI ranks in use.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 11, 2018

@rhalver does this work with the original gmake build of LAMMPS or only with the CMake? I tried gmake but the Makefile.lammps file in /lib/scafacos is missing and the build failed.

@@ -439,15 +439,15 @@ bool FixScafacos::check_result(FCSResult result, int comm_rank)
{
if (result)
{
printf("ScaFaCoS Error: Caught error on task %d.\n", comm_rank);
fprintf(stdout,"ScaFaCoS Error: Caught error on task %d.\n", comm_rank);

This comment has been minimized.

@akohlmey

akohlmey Jul 11, 2018

Member

no, this is just as bad as the original. please re-read my comment more carefully and compare to how this is handled by other code in LAMMPS.

This comment has been minimized.

@rhalver

rhalver Jul 11, 2018

Collaborator

Ok I think the easiest way is to remove the fprintf statement and just pass the internal error message to the error -> one() call. As it is possible that a subset of processes (>1) may encounter an error, is error -> one() still the correct call?

This comment has been minimized.

@stanmoore1

stanmoore1 Jul 11, 2018

Contributor

As it is possible that a subset of processes (>1) may encounter an error, is error -> one() still the correct call?

Yes this is fine. You can only use error->all if you can guarantee that all processes will reach it, otherwise it will deadlock.

rhalver and others added some commits Jul 18, 2018

added scaling for different units (lj, metal, ...) and storage of ato…
…m-wise potentials (eatom / eatom_flag)
must add USER-SCAFACOS to PACKLIB variable in Makefile
We need "make no-lib" to be correct for automated testing to work, since only known packages with libraries can be successfully compiled through jenkins and other tools. Those scripts usually do "make yes-all no-lib" and then "make yes-XXX" for selected packages where it is known how to build the required libs.
Steven J. Plimpton
#define LMP_SCAFACOS_H

#include "kspace.h"
#include "fcs.h"

This comment has been minimized.

@akohlmey

akohlmey Aug 22, 2018

Member

this statement must not be here. as commented before, we have to keep our namespace clean, which means all variables/pointers that require types from a header could be encapsulated into a pointer to a struct (for which we can put a forward declaration into the header) or should be cast to void pointers or stored in compatible regular types and then the header specific types can be used and the header included in the implementation, i.e. the .cpp file only. we had a bunch of issues because of namespace pollution already and it took some effort to get this cleaned up in almost all places where this was possible without having to rewrite large parts of the code (some parts like USER-REAXC, this was only done partially).

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 22, 2018

@rhalver - the issue Axel flagged is to remove the fcs.h file from scafacos.h, which also means
all the fcs and FCS declarations. I think fcs (handle) could just be void *fcs_handle, and converted back
to an FCS pointer when used in scafacos.cpp. Result could just be dropped from the *.h since you
could declare it whenever used in *.cpp. The fcs int/float could be regular int and double and the ScaF methods that use them could copy values in/out of them into locally declared fcs_int/float.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Aug 22, 2018

@rhalver

This comment has been minimized.

Copy link
Collaborator

rhalver commented Aug 22, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Aug 22, 2018

@rhalver

This comment has been minimized.

Copy link
Collaborator

rhalver commented Aug 22, 2018

With the last commit I removed fcs.h from scafacos.h and replaced the custom structures
with void*. Also I replaced the ScaFaCoS data types with int and double, which should be
the most common choices, but in principle it is possible to use other data types.. Is it possible to
run LAMMPS purely with float as it then make sense to provide the possibility
to run ScaFaCoS with float as well (with the use of fcs_float set to float)?

qqrd2e = force->qqrd2e;

if (!initialized) {
printf("DEBUG: %p\n",&fcs);

This comment has been minimized.

@akohlmey

akohlmey Aug 23, 2018

Member

please remove debug statements like these.

@dschwen dschwen referenced this pull request Aug 23, 2018

Merged

Fix compile warnings when building with pedantic flags #1077

0 of 4 tasks complete

@akohlmey akohlmey removed their assignment Aug 25, 2018

@rbberger
Copy link
Member

rbberger left a comment

Breaks library build, which requires -fPIC

@sjplimp

sjplimp approved these changes Sep 5, 2018

Copy link
Contributor

sjplimp left a comment

all looks fine - going to remove one file
that was really a note for Rene

@sjplimp sjplimp removed their assignment Sep 5, 2018

@rbberger rbberger merged commit a28990e into lammps:master Sep 5, 2018

5 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/kokkos_omp head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment