General requirements

[gassmoeller]

There will be more specific comments later on, but these are the big items I can think of right now, that would need to be addressed before a possible submission to CIG:

• pick a license (see https://choosealicense.com/ for help on the topic)
• just keeping a Makefile will not be enough to make the code compile on different platforms. Better alternatives are cmake (e.g. a tutorial here: http://derekmolloy.ie/hello-world-introductions-to-cmake/) or GNU autoconf (e.g. here: https://www.edwardrosten.com/code/autoconf/), let me know if you want help with setting up a simple CMake file
• a name ... for now FDPS_SPH might suffice
• some documentation, e.g. as a Readme.md file and/or a pdf manual. The minimum topics that should be discussed are:
  • how to compile the code,
  • how to start a model (e.g. I had to find out that I need to create the output folder myself before running the model),
  • how to change model parameters,
  • an explanation of the physics that are solved by the code,
  • an example model that does something interesting (I guess this is what happens at the moment when I just start the code?)
  • which publications are available that use this code (i.e. proper citations), e.g. as a CITATION file in the repository
• I would suspect that for your papers you ran tests and benchmarks that were published? It would be great if their input parameters could be part of the repository so that others can recreate the models.

[mikinakajimav2]

Output:

• It would be great if the manual has information about output. I am guessing it is

time (second)
Number of SPH particles
ID, tag(mantle=0, core=1), x, y, z, vx, vy, vz, density, energy, pressure, potential energy
[All SI unit]

• Does the output include information about the impactor?

Input:

• it seems that a user has to define dx, (init/GI.h) from which the code computes the number of SPH particles. I personally prefer giving the number of SPH particles as an input, and compute the dx.

I am adding some comments which were discussed but not listed above:

• What makes the code unique? (vs. Gadget etc)
• benchmark tests can be stored under test/ directory
• input files should be separate, so that you don't need to compile when you change inputs
• minimum document requirements: governing equations & how to use the code

[mikinakajimav2]

Overleaf document for the manual:
https://v2.overleaf.com/6318768974bmqxxjnwpbfw

[mikinakajima]

Some tasks:

• double check CMake
• double check .ignore
  mkdir build
  cmake ..
  make
  ln -s build/sph.out sph.out
• input.txt should include the EoS name

[mikinakajima]

Hi Rene,
This is what worked for me, but not for Tyler (he also has a Mac)

at FDPS_SPH
mkdir build
cd build
cmake .. -DCMAKE_CXX_COMPILER=g++-mp-8
(that generates a Makefile at FDPS_SPH) (edited)
cd ..
make
(which generates sph.out)
(either)
cp sph.out src/.
(or)
cd src
ln -s ../sph.out sph.out (edited)

However, for Tyler, the default compiler is Clang (which I also encountered the same issue)
when he tried to use g++, he gets

(base) vpn13-103:build Tyler-EES$ cmake -DCMAKE_CXX_COMPILER=g++ ..
-- ====================================================
-- ============ Configuring FDPS_SPH ==================
-- ====================================================
-- The C compiler identification is unknown
-- The CXX compiler identification is AppleClang 10.0.1.10010046
-- Check for working C compiler: /opt/local/bin/g++-mp-8
-- Check for working C compiler: /opt/local/bin/g++-mp-8 -- broken
CMake Error at /usr/local/Cellar/cmake/3.14.5/share/cmake/Modules/CMakeTestCCompiler.cmake:60 (message):
The C compiler

“/opt/local/bin/g++-mp-8”

is not able to compile a simple test program.

It fails with the following output:

Change Dir: /Users/Tyler-EES/Documents/GitHub/FDPS_SPH/build/CMakeFiles/CMakeTmp

Run Build Command(s):/usr/bin/make cmTC_7effc/fast
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f CMakeFiles/cmTC_7effc.dir/build.make CMakeFiles/cmTC_7effc.dir/build
Building C object CMakeFiles/cmTC_7effc.dir/testCCompiler.c.o
/opt/local/bin/g++-mp-8 -o CMakeFiles/cmTC_7effc.dir/testCCompiler.c.o -c /Users/Tyler-EES/Documents/GitHub/FDPS_SPH/build/CMakeFiles/CMakeTmp/testCCompiler.c
/Users/Tyler-EES/Documents/GitHub/FDPS_SPH/build/CMakeFiles/CMakeTmp/testCCompiler.c:2:3: error: #error “The CMAKE_C_COMPILER is set to a C++ compiler”
# error “The CMAKE_C_COMPILER is set to a C++ compiler”
^~~~~
make[1]: *** [CMakeFiles/cmTC_7effc.dir/testCCompiler.c.o] Error 1
make: *** [cmTC_7effc/fast] Error 2

CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
CMakeLists.txt:8 (PROJECT)

-- Configuring incomplete, errors occurred!
See also “/Users/Tyler-EES/Documents/GitHub/FDPS_SPH/build/CMakeFiles/CMakeOutput.log”.
See also “/Users/Tyler-EES/Documents/GitHub/FDPS_SPH/build/CMakeFiles/CMakeError.log”.

When he tried to compile with g++-mp-8, he gets

-- The C compiler identification is unknown
-- The CXX compiler identification is GNU 8.3.0
-- Check for working C compiler: /opt/local/bin/g++-mp-8
-- Check for working C compiler: /opt/local/bin/g++-mp-8 -- broken

[gassmoeller]

Hi Miki,
based on your description it seems like you and Tyler have different compilers / build environments installed on your Macs. Does the simple Makefile that is already in the src/ directory work for him?

I found a blog post that discusses using OpenMP on Mac, maybe this is of some help: https://iscinumpy.gitlab.io/post/omp-on-high-sierra/

[mikinakajima]

Yes, the simple make file using g++-mp-8 as a compile worked for Tyler too

[mikinakajima]

I think its problematic that some FDPS versions are not compatible with the current SPH code. I thought we could upload a compatible FDPS version on git when we release FDPS-SPH. Is there any better way to do this?

[TylerLaBree]

I'm not sure what exactly fixed it, but something Miki or I changed fixed CMake on my end.

[gassmoeller]

@mnakajima7: There is a better way. We can specify certain version of FDPS as safe and automatically install one of them. That way we do not need to duplicate the files into this repo. Do you have a working version, and can you let me know which git commit hash that has? (you can find out by git log). I am currently using v4.1a (hash: 23c1b1baca5aad5cd8ba94312e534c859db4e91e) and that seems to work.

[gassmoeller]

@TylerLaBree: That is good to hear. Maybe you installed a compiler with OpenMp support?

[mikinakajima]

@gassmoeller That's a great idea! Currently I am using 5.0c FDPS
https://github.com/FDPS/FDPS/releases/tag/v5.0c
I am not quite sure how to find the corresponding hash... should I try to find the commit when I was using a functional FDPS? I did not find any commit between Jan 24 (v5.0c released) and Feb 28 (v5.0d released). Let me know if I completely missed something

[NatsukiHosono]

@mnakajima7
Sorry for my delay, I'm in the middle of an International Conference.
I haven't caught up, but in the upcoming version of FDPS would resolve the problem.
I think you can find hashes of older versions of FDPS from https://github.com/FDPS/FDPS/tags .

[TylerLaBree]

@gassmoeller @NatsukiHosono
It looks like it's on the to-do list to allow command line options to specify the file for the initial conditions of the simulation. Currently, argc and argv[] look like they're being used to ask if it should resume an old simulation from the specified timestep.

Would you recommend I amend this to include the option to specify input file, in addition to output directory location? This would allow us to easily run multiple simulations at once from different input files, and with different output directories.

If so, do you have any advice on best practice for this, so I don't break old functionality?

[NatsukiHosono]

@TylerLaBree
Yes, for now, the command line options are only used to specify whether it is a new run or not.
This is a temporal way, so if you have a more useful way, you can amend it.

[gassmoeller]

Yes I think it would be great to have more flexibility in selecting input files. As an example in ASPECT we handle command-line arguments the following way:

  // Loop over all command line arguments. Handle a number of special ones
  // starting with a dash, and then take the first non-special one as the
  // name of the input file. We will later check that there are no further
  // arguments left after that (though there may be with PETSc, see
  // below).
  while (current_argument<argc)
    {
      const std::string arg = argv[current_argument];
      ++current_argument;
      if (arg == "--output-xml")
        {
          output_xml = true;
        }
      else if (arg == "--output-plugin-graph")
        {
          output_plugin_graph = true;
        }
      else if (arg=="-h" || arg =="--help")
        {
          output_help = true;
        }
      else if (arg=="-v" || arg =="--version")
        {
          output_version = true;
        }
      else if (arg=="-j" || arg =="--threads")
        {
#ifdef ASPECT_USE_PETSC
          std::cerr << "Using multiple threads (using -j) is not supported when using PETSc for linear algebra. Exiting." << std::endl;
          return -1;
#else
          use_threads = true;
#endif
        }
      else if (arg == "--test")
        {
          run_unittests = true;
          break;
        }
      else if (arg == "--validate")
        {
          validate_only = true;
        }
      else
        {
          // Not a special argument, so we assume that this is the .prm
          // filename (or "--"). We can now break out of this loop because
          // we are not going to parse arguments passed after the filename.
          prm_name = arg;
          break;
        }
    }

Something similar would be reasonable I think. @NatsukiHosono how attached are you to the current functionality that a single argument is always interpreted as the timestep for a restart? Would it be ok to make that an option, e.g. instead of ./FDPS_SPH 5000 say ./FDPS_SPH --restart 5000? That would be easier to parse and distinguish from a parameter file, and more intuitive for users.

[TylerLaBree]

@gassmoeller, This sounds like a great idea! I think I've got a decent version of this running now. So far, the user can specify an input file with -i or --input, an output directory with -o or --output, and a time-step to start on with -r or --resume. Is resume an appropriate name for this option?

@NatsukiHosono, Which would you prefer between those options? Would you like the user to specify resuming/restarting a sim from a specified time-step using the terminal, or from the input file specified? Thanks!

[NatsukiHosono]

@gassmoeller @TylerLaBree Well, I think it would be better to specify the resuming timestep from the command line options, e.g., --restart 5000.

Actually, there was a reason why I adopt such a simple way to resume a run.
Originally, this code was developed to run on the Japanese supercomputer, K computer.
It has, however, a very inconvenient job scheduler so I'd like to keep my code as simple as possible.
But the K computer is supposed to be removed within a few months.
So I have no reason to stick to such a rough resuming any more.

[TylerLaBree]

Sounds good @NatsukiHosono !

Another question: I'm looking into adding an option for velocity damping, so we can allow the target and impactor to individually come into equilibrium before we simulate a collision. How would you recommend doing this in the correct way? I'm trying to familiarize myself with the force calculation that happens every step, but it's complex and I'm not sure which thing I should edit.

[NatsukiHosono]

@TylerLaBree In my way, we should write a subroutine to implement non-interparticle forces.
For example, addExternalForce in init/GI.h (line 248).
In this routine, a damping term is added on each particle on every step.
I hope this helps!