search

ls1mardyn / ls1-mardyn

ls1-MarDyn is a massively parallel Molecular Dynamics (MD) code for large systems. Its main target is the simulation of thermodynamics and nanofluidics. ls1-MarDyn is designed with a focus on performance and easy extensibility.
http://www.ls1-mardyn.de
Other
28 stars 15 forks source link

Reflecting boundaries #339

Open amartyads opened 1 month ago

amartyads commented 1 month ago

Description

Added "native" support for reflecting and outflow boundaries in ls1.

The actual communication code was left untouched, and the boundary effects were manages with these two steps:

The communication code was not changed, because I did not want to mess up the inter-subdomain communication for internal molecules.

How Has This Been Tested?

Since this adds more checks for default (periodic) behaviour, I ran a test on the AMD minicluster to make sure that default behaviour is not slower.

configForBoundary.txt

jobForBoundary.txt

50000 steps, 485150 particles

I ran each run 5 times, took time values from the output files, averaged them, and here are the results (in seconds):

Container Run Type Wall time Boundary Effects Time Force Computation Time Total Computation Time Communication Time
AP master 746.116 0.000000 548.0748 582.2928 132.6998
periodic 744.350 0.090808 544.4254 578.4000 134.4734
reflecting 737.930 15.541120 542.4348 586.7612 124.2872
LC master 1413.200 0.000000 735.0464 885.0394 282.4636
periodic 1404.534 0.149090 737.6524 880.9132 276.3130
reflecting 1486.748 147.659200 713.3886 887.7828 267.289

So compared to master, running periodic boundaries on this branch seems to have negligible overhead, hence performance should not be massively affected overall.

Otherwise, this has been more or less extensively tested, both with and without MaMiCo, so everything should be fine.

amartyads commented 1 month ago

NB: can only be merged after merging #293 because I needed those fixes in this branch to test it. Hence, this is only draft for now, and will be turned to full after that PR is merged.

FG-TUM commented 1 month ago

NB: can only be merged after merging #293 because I needed those fixes in this branch to test it. Hence, this is only draft for now, and will be turned to full after that PR is merged.

Then it would make sense if this PR has the branch of #293 (mamico-postrefactor-fix) as target and not master. As soon as #293 is merged, this PR's target is automatically changed to master.

amartyads commented 1 month ago

@FG-TUM I'm not sure why this PR is showing ResultWriter.* and Domain.* as changed files. The changes in those files are already in master under #336 . All the branches have pulled and merged master, but it still shows those files as being changed.

FG-TUM commented 1 month ago

@FG-TUM I'm not sure why this PR is showing ResultWriter.* and Domain.* as changed files. The changes in those files are already in master under #336 . All the branches have pulled and merged master, but it still shows those files as being changed.

Hmm weird. This is a known bug on GitHub but I don't really know what triggers it. What exactly was your merge command / strategy (and ~/.gitconfig)?

amartyads commented 1 month ago

@FG-TUM I'm not sure why this PR is showing ResultWriter.* and Domain.* as changed files. The changes in those files are already in master under #336 . All the branches have pulled and merged master, but it still shows those files as being changed.

Hmm weird. This is a known bug on GitHub but I don't really know what triggers it. What exactly was your merge command / strategy (and ~/.gitconfig)?

I just used git pull and git merge master for the branches, and just the default strategy (which I assume is ort), no rebasing or squashing or anything

my .gitconfig just has my email and user, my editor, and my github API keys, nothing else, it's very weird

amartyads commented 3 weeks ago

@HomesGH Do you want to take a look at the comments Fabio left in the files that were part of #336 ?

HomesGH commented 3 weeks ago

@HomesGH Do you want to take a look at the comments Fabio left in the files that were part of #336 ?

I went through the comments in the files Domain. and ResultWriter. and resolved them. However, I have no idea about the following section

double _universalProfiledComponentMass;  // set from outside
double _universalLambda;  // set from outside
float _globalDecisiveDensity;  // set from outside
amartyads commented 3 weeks ago

@HomesGH Do you want to take a look at the comments Fabio left in the files that were part of #336 ?

I went through the comments in the files Domain. and ResultWriter. and resolved them. However, I have no idea about the following section

double _universalProfiledComponentMass;  // set from outside
double _universalLambda;  // set from outside
float _globalDecisiveDensity;  // set from outside

I have no idea either, unfortunately

Checking git history, seems like those lines have existed unchanged since 2012 (commit 9b1e25174)

amartyads commented 3 weeks ago

After all these changes I redid the performance test, and the results don't seem impacted at all (I didn't rerun master):

Container Run Type Wall time Boundary Effects Time Force Computation Time Total Computation Time Communication Time
AP master 746.116 0.000000 548.0748 582.2928 132.6998
periodic 740.762 0.164010 543.6474 578.2006 131.9316
reflecting 739.328 0.106152 541.3684 576.2200 132.5192
LC master 1413.200 0.000000 735.0464 885.0394 282.4636
periodic 1403.222 0.292358 722.1366 866.5802 282.3060
reflecting 1404.236 0.178882 731.8196 874.3370 280.8740

LC (base ls1) seems less consistent somehow, maybe last time's runs got skewed by one bad run (possible because I'm testing on the minicluster).

But in either case, no slowdowns from all the corrections.