search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: PAPRECA: A parallel hybrid off-lattice kinetic Monte Carlo/molecular dynamics simulator #6714

Closed editorialbot closed 5 months ago

editorialbot commented 7 months ago

Submitting author: !--author-handle-->@sntioudis<!--end-author-handle-- (Stavros Ntioudis) Repository: https://github.com/sntioudis/papreca Branch with paper.md (empty if default branch): Version: v1.0.1 Editor: !--editor-->@srmnitc<!--end-editor-- Reviewers: @liamhuber, @jfaraudo Archive: 10.5281/zenodo.11550493

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/f00ac3f3856e2f369c96646b66a1581b"><img src="https://joss.theoj.org/papers/f00ac3f3856e2f369c96646b66a1581b/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/f00ac3f3856e2f369c96646b66a1581b/status.svg)](https://joss.theoj.org/papers/f00ac3f3856e2f369c96646b66a1581b)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@liamhuber & @jfaraudo, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @srmnitc know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @liamhuber

📝 Checklist for @jfaraudo

editorialbot commented 7 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 7 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.commatsci.2023.112421 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.1103/PhysRevB.75.205339 is OK
- 10.1021/acs.jctc.1c00921 is OK
- 10.1021/cr400234a is OK
- 10.1063/1.1369622 is OK
- 10.1063/1.2919546 is OK
- 10.1016/j.jcp.2015.12.001 is OK
- 10.1021/cs3005709 is OK
- 10.1007/978-3-319-44677-6_29 is OK
- 10.1039/C6NR01881E is OK
- 10.1016/j.triboint.2018.02.005 is OK
- 10.1063/1.461138 is OK
- 10.1016/0021-9991(76)90041-3 is OK
- 10.21105/joss.00247 is OK
- 10.21105/joss.01168 is OK
- 10.21105/joss.02307 is OK
- 10.21105/joss.02307 is OK
- 10.1088/1361-651X/accc4b is OK
- 10.1016/j.cpc.2014.04.017 is OK
- 10.3390/min10090825 is OK
- 10.1063/1.5046635 is OK
- 10.1088/0965-0393/22/5/055002 is OK
- 10.1063/1.1415500 is OK
- 10.3389/fchem.2019.00202 is OK

MISSING DOIs

- No DOI given, and none found for title: kmcos: Kinetic Monte Carlo of Systems
- No DOI given, and none found for title: Python-based Charge Dynamics (PyCD)
- No DOI given, and none found for title: VIS-A-VIS: agent-based simulator of viral infectio...
- No DOI given, and none found for title: MulSKIPS: A Kinetic Monte Carlo super-Lattice code
- No DOI given, and none found for title: Kimocs - Kinetic Monte Carlo for Surfaces
- No DOI given, and none found for title: KSOME: Kinetic Simulations of Microstructural Evol...
- No DOI given, and none found for title: kMCpy: Kinetic Monte Carlo Simulation using Python
- No DOI given, and none found for title: Morphokinetics

INVALID DOIs

- None
editorialbot commented 7 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.05 s (1532.4 files/s, 265822.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             20           2142           1597           2983
Markdown                        11            689              0           1213
C/C++ Header                    19            522            617            938
Python                           7            232            103            634
TeX                              1             32              0            377
CMake                            2             36             20            152
YAML                             3             24             13            113
Bourne Shell                     8             27             17             82
make                             2             29             19             52
-------------------------------------------------------------------------------
SUM:                            73           3733           2386           6544
-------------------------------------------------------------------------------

Commit count by author:

   107  Stavros Ntioudis
     5  James Ewen
     1  sntioudis
editorialbot commented 7 months ago

Paper file info:

📄 Wordcount for paper.md is 1317

✅ The paper includes a Statement of need section

editorialbot commented 7 months ago

License info:

🟡 License found: GNU General Public License v2.0 (Check here for OSI approval)

editorialbot commented 7 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

srmnitc commented 7 months ago

👋🏼 @sntioudis @liamhuber @jfaraudo this is the review thread for the paper. All of our communications will happen here from now on.

As a reviewer, the first step is to create a checklist for your review by entering

@editorialbot generate my checklist

as the top of a new comment in this thread.

These checklists contain the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#REVIEW_NUMBER so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use EditorialBot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@srmnitc ) if you have any questions/concerns, thanks again for the submission, and for thr reviews

liamhuber commented 6 months ago

Review checklist for @liamhuber

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

liamhuber commented 6 months ago

Write-up comment:

Overall, kMC techniques are, on one hand, less accurate, but on the other hand, more computationally efficient than other atomistic techniques such as the density functional theory (DFT) or molecular dynamics (MD).

I find the comparison to DFT to be apples-to-oranges, it's equivalent to saying "more computationally efficient that MEAM potentials". DFT is a tool for transforming structures (position+chemistry) into energies and forces (and electron density); kMC is a tool for evolving structures in time. In contrast, the comparison to MD is apples-to-apples. Indeed, one could use DFT (assuming arbitrarily powerful compute) and MD and kMC all together! So the real contrast here is kMC vs brute-force MD for temporal evolution.

IMO the comparison to brute-force MD is already sufficient and DFT should simply be removed. If you really want multiple items to contrast, you could consider mentioning some other acceleration technique, e.g. stuff by Danny Perez and Arthur Voter comes to mind.

liamhuber commented 6 months ago

Write-up comment:

Ah, sorry, I see the paper itself is also on the repo. I'll transfer this to an issue there.

liamhuber commented 6 months ago

~Blocking: https://github.com/sntioudis/papreca/issues/9~ closed

srmnitc commented 6 months ago

@jfaraudo just a short reminder. Do you need anything from my side to get started?

jfaraudo commented 6 months ago

@srmnitc sorry for the delay, I am having very busy days with the end of term and several grant deadlines. I'll start to work on this later today or tomorrow.

srmnitc commented 6 months ago

@srmnitc sorry for the delay, I am having very busy days with the end of term and several grant deadlines. I'll start to work on this later today or tomorrow.

Thanks for the update!

sntioudis commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jfaraudo commented 6 months ago

Review checklist for @jfaraudo

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

liamhuber commented 6 months ago

~Blocking: https://github.com/sntioudis/papreca/issues/12~ closed

liamhuber commented 6 months ago

~Blocking: https://github.com/sntioudis/papreca/issues/13~

sntioudis commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

liamhuber commented 6 months ago

Update from my end

Everything is looking super good so far. @sntioudis thanks for the quick responses to all my issues and for jumping into the software engineering/devops side of things with both feet! Even most of my non-blocking issues have been resolved and I appreciate the time and effort you've put in to working with GitHub's workflow platform to accomplish this.

The last thing I want to do is try something "new" with the software. I'll also use whatever little toy run I come up with to try and verify the scaling behaviour; I only have access to my desktop machine (8 cores) -- I verified that the scaling example runs OK, but on 4 cores it took overnight to execute so I don't want to repeat that a bunch of times 😂 I'll make sure I wrap that up by the end of next week 👍

Disclaimers to my review

srmnitc commented 6 months ago

@liamhuber Thanks for the update, and thanks @sntioudis for fixing all the issues that came up!

liamhuber commented 6 months ago

@srmnitc, I'm done and satisfied. @sntioudis, :rocket: nice work!

I made my own very small scaling test and was only able to run on 1,2,4 cores, but it looks qualitatively similar to the paper figure. The key thing is that the expensive atomic calculations leverage all the parallelization of lammps, and that's working a-ok.

My "new" thing was not mindblowing, but I had fun. I wanted to explore Ostwald ripening, so I modified the brownian motion example to include a minimization (full inputs collapsed below). The diffusive hops are a little bit aphysical for this purpose, but I suppose one could very roughly think of it as interacting particles moving around in a non-interacting, implicit, amorphous medium -- i.e. the particles need to wait for the medium to provide diffusion opportunities but don't impact the energetics. I still expect Ostwald ripening under these conditions even just because of the surface-area-to-volume statistics about who's most likely to lose a % of itself to the "gas".

Papreca input ``` #PAPRECA input file #Basic settings KMC_steps 4000000 KMC_per_MD 100 random_seed 13452234 fluid_atomtypes 1 1 #Sigmas for types sigmas_options LAMMPS mix geom #Equilibration trajectory_duration 1 minimize_prior yes minimize 1.0e-16 1.0e-16 100 20000 #Predefined events create_DiffusionHop 1 0.0 3 yes 1 rate_manual 1.0 random_diffvecs yes 3D #NeibLists neiblists lj/cut zero ```
Lammps input ``` #LAMMPS input file #---------------------------Simulation parameters-------------------------- dimension 3 units real atom_style full pair_style hybrid/overlay lj/cut 10 zero 10 full #---------------------------Processors------------------------------------- processors * * * #---------------------------Define boundaries------------------------------ boundary p p p #--------------------------Create simulation------------------------------- region simulation_box block -50 50 -50 50 -50 50 create_box 1 simulation_box #--------------------------Create atoms------------ create_atoms 1 random 1000 141241 NULL #---------------------------Atom masses------------------------------------ mass 1 1.0079 #----------------------Pair coeffs ------------------------- pair_coeff 1 1 lj/cut 0.0152 2.84642 pair_coeff * * zero #----------------------Define group and neib/charge interactions----------- neighbor 2 bin neigh_modify every 1 delay 0 check yes group fluid type 2 group frozen type 1 #----------------Set Timestep---------------------------------------------- timestep 1.0e-100 #----------------------File Outputs---------------------------------------- dump 1 all xyz 404 ostwald.xyz dump_modify 1 element H first yes ```

I was a little surprised to see my particles come out as needles rather than spheroids, but it's probably just something to do with the LJ trying to pack itself amorphously and I'm not stressed about it. Overall the ripening proceeded as expected and I had fun:

Initial: start

Early: early

Mid: mid

Late: late

Some concluding thoughts:

srmnitc commented 6 months ago

@liamhuber Thanks for lot for this extensive review and trying out the package! I think your comments have been super helpful for @sntioudis.

srmnitc commented 6 months ago

@jfaraudo Just a quick reminder from my side, would you be able to get started with the review soon? Thanks again for taking your time!

jfaraudo commented 6 months ago

@srmnitc Yes, I have to reinstall LAMMPS in my machine since I have a version older than that recommended for the build of PAPRECA (LAMMPS is required as a library). I am unsure to do the the minimal LAMMPS installation required for PAPRECA or my usual build of LAMMPS which adds more features not needed in PAPRECA. Perhaps I'll consider using a fresh spare computer for testing. Sorry again for that delay.

sntioudis commented 6 months ago

@liamhuber really thank you for your time and your commitment to this review! Thanks to your suggestions, the software and the paper have significantly improved over the past few weeks. I also had good fun working on the repository and I learned a lot! Furthermore, thank you for using papreca to study something different than the documented examples. It was a joy to see the software producing sensible results for a system that has never been studied before with papreca.

It's nice that the phosphate scaling test is directly available, but when getting started and running through all the examples it came as a nasty shock to go from the example finishing in a few seconds to hours -- maybe break it into a separate folder or scale down the run-time in the default script?

This is a fair point. I will add a disclaimer to the documentation page to warn users that the phosphate examples require considerably more time to finish than the rest of the examples. I will also test if it is possible to reduce the trajectory duration and still obtain sensible growth results.

Extending the docs with atomistic schematic diagrams for the different event types would be cool.

Another good point. I will add a few images to the documentation (probably in the "Essential Information" or the "PAPRECA Commands" section).

I had no trouble getting the trajectory file to dump how I wanted, but my log.lammps files got huge -- is there a way to control this from the lammps or papreca inputs? It's been years since I worked with the lammps input script directly, so it's highly probably that I'm just being a big dummy here, but the world is full of dummies and some hand-holding documentation on controlling output file size (even if it's just regurgitating lammps docs) might be helpful

Adding the following line to your LAMMPS input prevents LAMMPS from constantly writing on the log.lammps file:

`log none'

I tried adding this line to the end of your LAMMPS input (Oswald ripening example) and the size of the output log.lammps file was reduced to just 1.7 kb.

I will soon add a note in the documentation to encourage users to include the 'log none' option in their papreca simulations to prevent log.lammps files from getting very large!

EDIT: I made the relevant changes to the documentation. See here and here.

sntioudis commented 6 months ago

@jfaraudo

Yes, I have to reinstall LAMMPS in my machine since I have a version older than that recommended for the build of PAPRECA (LAMMPS is required as a library). I am unsure to do the the minimal LAMMPS installation required for PAPRECA or my usual build of LAMMPS which adds more features not needed in PAPRECA. Perhaps I'll consider using a fresh spare computer for testing. Sorry again for that delay.

I would be happy to help you tackle your installation issues if you wish :)! Just let me know which version of LAMMPS (and LAMMPS packages) you are using and what sort of installation/build errors you are facing.

srmnitc commented 6 months ago

@srmnitc Yes, I have to reinstall LAMMPS in my machine since I have a version older than that recommended for the build of PAPRECA (LAMMPS is required as a library). I am unsure to do the the minimal LAMMPS installation required for PAPRECA or my usual build of LAMMPS which adds more features not needed in PAPRECA. Perhaps I'll consider using a fresh spare computer for testing. Sorry again for that delay.

Thanks for the remarks. Another option could be to use the conda package lammps with conda install -c conda-forge lammps.

jfaraudo commented 6 months ago

I did a fresh install of LAMMPS with cmake and it is now up to date . But I am still having problems installing PAPRECA with cmake in my desktop ubuntu workstation, apparently a conflict with anaconda libraries. I think I'll try with another computer and see what happens. This is the error, just in case the solution is obvious to anybody

-- Configuring done (0.6s) CMake Warning at CMakeLists.txt:51 (add_executable): Cannot generate a safe runtime search path for target papreca because files in some directories may conflict with libraries in implicit directories:

runtime library [libpng16.so.16] in /usr/lib/x86_64-linux-gnu may be hidden by files in:
  /home/jfaraudo/anaconda3/lib
runtime library [libz.so.1] in /usr/lib/x86_64-linux-gnu may be hidden by files in:
  /home/jfaraudo/anaconda3/lib
runtime library [libgomp.so.1] in /usr/lib/gcc/x86_64-linux-gnu/7 may be hidden by files in:
  /home/jfaraudo/anaconda3/lib

Some of these libraries may not be found correctly.

-- Generating done (0.0s) -- Build files have been written to: /home/jfaraudo/source_programs/papreca-main/build

sntioudis commented 6 months ago

@jfaraudo I am not 100% sure but it appears that there are duplicate installations of certain shared libraries in your system. This issue is discussed here. This looks like a warning (rather than an error), which means that building the project might work (however, the produced executable might lead to unpredictable behavior - so it is better to fix that before testing papreca). Also, I noticed that there are conflicts between the 'user/lib/' and 'anaconda3/lib' paths which might be a conda issue (e.g., some of those libraries were installed on the conda 'base' environment or there were conflicts on the conda environment you used to install papreca? Not sure about that, just an idea).

In the provided link above, one of the suggestions was to uninstall one of the two library installations (located in the 2 different locations indicated by your error). Another solution might be to provide cmake with a hint, so it targets a specific library location.

Of course, I understand that tackling the warning and uninstalling libraries might be harder than installing LAMMPS and PAPRECA on another computer. In any case, keep me updated and I will be very happy to assist you further if neccessary!

jfaraudo commented 6 months ago

I have added a few comments about the paper in this issue: https://github.com/sntioudis/papreca/issues/15

jfaraudo commented 6 months ago

@sntioudis I have tried a fresh install of LAMMPS and papreca on a new machine (with fedora Linux) . LAMMPS install was OK but I had errors with papreca. So I opened that issue: https://github.com/sntioudis/papreca/issues/16

jfaraudo commented 6 months ago

@sntioudis I have tried a fresh install of LAMMPS and papreca on a new machine (with fedora Linux) . LAMMPS install was OK but I had errors with papreca. So I opened that issue: sntioudis/papreca#16

Installed and working, now I'll proceed to test the examples.

As a suggestion for the documentation, I think it is better to add as recommendation the procedure that solved my compilation problems. Locate first your mpicxx using which mpicxx and then use this path in the cmake with the option -DCMAKE_CXX_COMPILER=/path/to/your/mpicxx (in my case /usr/lib64/openmpi/bin/mpicxx )

Once this is included in the documentation, the issue #16 can be closed https://github.com/sntioudis/papreca/issues/16

jfaraudo commented 6 months ago

I am now testing the three examples provided with the Software. I have a few comments about them, that I am posting in a new issue, starting from the Brownian motion elementary example. https://github.com/sntioudis/papreca/issues/17

jfaraudo commented 6 months ago

@sntioudis I have tried a fresh install of LAMMPS and papreca on a new machine (with fedora Linux) . LAMMPS install was OK but I had errors with papreca. So I opened that issue: sntioudis/papreca#16

Installed and working, now I'll proceed to test the examples.

As a suggestion for the documentation, I think it is better to add as recommendation the procedure that solved my compilation problems. Locate first your mpicxx using which mpicxx and then use this path in the cmake with the option -DCMAKE_CXX_COMPILER=/path/to/your/mpicxx (in my case /usr/lib64/openmpi/bin/mpicxx )

Once this is included in the documentation, the issue #16 can be closed sntioudis/papreca#16

@sntioudis I see that you closed this issue but the comment about cxx_compiler is still not included in the documentation as suggested (maybe it is uploading or something?)

sntioudis commented 6 months ago

• After configuring the LAMMPS build, kindly ensure that the MPI package has been correctly configured. If the MPI package is not configured, cmake will build a non-MPI LAMMPS version which will lead to PAPRECA linkage errors. If the installed MPI compiler in your system is not detected by cmake, you should add the following argument to your cmake configuration command: -DCMAKE_CXX_COMPILER=/path/to/your/cxx/compiler. Hint: the relevant cxx compiler path can be obtained by running: 'which compiler_name' (e.g., 'which mpicxx'). It is advised that the same flag (i.e., -DCMAKE_CXX_COMPILER) is also used for the configuration of LAMMPS.

@jfaraudo This comment (third bullet point under the LAMMPS installation instructions) appears on my browser. Does it appear when you refresh?

jfaraudo commented 6 months ago

@srmnitc I finished executing the examples and revising the documentation. Overall, I like the work , @sntioudis you did a great job. I have a few comments and suggestions, I'll post all of them asap.

jfaraudo commented 6 months ago

• After configuring the LAMMPS build, kindly ensure that the MPI package has been correctly configured. If the MPI package is not configured, cmake will build a non-MPI LAMMPS version which will lead to PAPRECA linkage errors. If the installed MPI compiler in your system is not detected by cmake, you should add the following argument to your cmake configuration command: -DCMAKE_CXX_COMPILER=/path/to/your/cxx/compiler. Hint: the relevant cxx compiler path can be obtained by running: 'which compiler_name' (e.g., 'which mpicxx'). It is advised that the same flag (i.e., -DCMAKE_CXX_COMPILER) is also used for the configuration of LAMMPS.

This comment (third bullet point under the LAMMPS installation instructions) appears on my browser.

OK, my browser maintains the old version in cache , I see the updated version now in a different browser. OK!

jfaraudo commented 6 months ago

I added another Issue for example 2 for clarity https://github.com/sntioudis/papreca/issues/18 So previous issue #17 https://github.com/sntioudis/papreca/issues/17 refers only to Example 1.

srmnitc commented 6 months ago

@srmnitc I finished executing the examples and revising the documentation. Overall, I like the work , @sntioudis you did a great job. I have a few comments and suggestions, I'll post all of them asap.

Thanks a lot for your efforts @jfaraudo !

jfaraudo commented 6 months ago

@srmnitc @sntioudis I closed the issues related to the Examples. I am fully satisfied with the corrections and updates made by @sntioudis to the Examples. I especially like the fact that Example 3 corresponds to an actual research paper. The selection of examples is great. Example 1 teaches you how to work with this software. Example 2 shows a classical example that can be compared with exact analytical solutions. And finally example 3 shows a Research paper result. I have a few minor comments about the paper itself and the documentation that are posted in the following issues: https://github.com/sntioudis/papreca/issues/15 https://github.com/sntioudis/papreca/issues/19 I think that, after consideration of these comments, the work can be published.

srmnitc commented 6 months ago

@jfaraudo @liamhuber Thanks once again for your detailed reviews. @sntioudis Please let me know once you close the last issues and then we can go ahead with rest of the process.

sntioudis commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jfaraudo commented 6 months ago

@srmnitc From my side, the paper is ready to be published. @sntioudis Thank you for your efforts, nice work.

sntioudis commented 5 months ago

I have a few minor comments about the paper itself and the documentation that are posted in the following issues: https://github.com/sntioudis/papreca/issues/15 https://github.com/sntioudis/papreca/issues/19 I think that, after consideration of these comments, the work can be published

Hi @srmnitc. The issues above have been resolved.

srmnitc commented 5 months ago

@sntioudis perfect thanks, we can move ahead with the remaining steps.

srmnitc commented 5 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance