search

openjournals / joss-reviews

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

[REVIEW]: OpenMD: A parallel molecular dynamics engine for complex systems and interfaces #7004

Open editorialbot opened 1 month ago

editorialbot commented 1 month ago

Submitting author: !--author-handle-->@gezelter<!--end-author-handle-- (J. Daniel Gezelter) Repository: https://github.com/OpenMD/OpenMD/ Branch with paper.md (empty if default branch): Version: v3.1 Editor: !--editor-->@srmnitc<!--end-editor-- Reviewers: @samwaseda, @blackspur Archive: Pending

Status

status

Status badge code:

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

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

@samwaseda & @blackspur, 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 @samwaseda

📝 Checklist for @blackspur

editorialbot commented 1 month 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 1 month ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.78 s (1357.9 files/s, 282969.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                            447          19065          27935          94351
C/C++ Header                   560           9088          31898          31539
CMake                           14            199            278           1749
Python                           9            225            154           1011
Markdown                        21            324              0            916
ANTLR Grammar                    2            143             52            799
TeX                              1              0              0            327
Perl                             1             63            105            239
YAML                             3             23             21            103
Bourne Shell                     1             12             20             48
-------------------------------------------------------------------------------
SUM:                          1059          29142          60463         131082
-------------------------------------------------------------------------------

Commit count by author:

  1574  Dan Gezelter
   426  Teng Lin
   316  Charles Vardeman
   199  Christopher Fennell
   171  Patrick Louden
    61  Hematna Bhattarai
    58  crdrisko
    33  Cody Drisko
    27  Xiuquan Sun
    25  Joseph Michalka
    14  Hemanta-Bhattarai
    14  Shenyu Kuang
    12  Soren Holm
    11  Chunlei Li
    11  Kelsey Stocker
     9  Hemanta
     9  James Marr
     8  Sydney Shavalier
     7  Hemanta Bhattarai
     7  Kyle Daily
     6  Christie Puglis
     6  Cody R. Drisko
     6  sshavali
     5  Anderson
     4  Hythem Sidky
     4  Martin Vala
     3  123manudai
     3  C.R. Drisko
     3  OpenMD
     3  Patrick-Louden
     3  adasilv3
     2  Kenneth Fletcher
     2  gezelter
     1  Alexander Joseph Mazanek
     1  Alexander Mazanek
     1  Anderson Da Silva Duraes
     1  Benjamin Harless
     1  Cody Ryan Drisko
     1  Gianluca Puliti
     1  Julien Nabet
     1  Madan Lamichhane
     1  Minh Nhat Pham
editorialbot commented 1 month ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/1.439486 is OK
- 10.1145/235815.235821 is OK
- 10.1103/PhysRevB.99.094106 is OK
- 10.1103/PhysRevB.29.6443 is OK
- 10.1021/acs.jctc.4c00182 is OK
- 10.1145/174462.156635 is OK
- 10.1103/physreva.34.2499 is OK
- 10.1007/BF00977785 is OK
- 10.1063/1.473271 is OK
- 10.1021/ja01299a050 is OK
- 10.1021/j100161a070 is OK
- 10.1063/1.468398 is OK
- 10.1063/1.480502 is OK
- 10.1021/ct100670m is OK
- 10.1063/1.478738 is OK
- 10.1021/jp025949h is OK
- 10.1063/1.2206581 is OK
- 10.1063/1.4896627 is OK
- 10.1063/1.4896628 is OK
- 10.1063/1.4960957 is OK
- 10.1002/jcc.20161 is OK
- 10.1080/00268976.2012.680512 is OK
- 10.1103/PhysRevE.59.4894 is OK
- 10.1063/1.3276454 is OK
- 10.1063/1.3499947 is OK
- 10.1021/ct500221u is OK
- 10.1038/sdata.2016.18 is OK

MISSING DOIs

- No DOI given, and none found for title: Sur la sphère vide: A la mémoire de Georges Vorono...
- No DOI given, and none found for title: Point multipoles in the Ewald summation
- No DOI given, and none found for title: Point multipoles in the Ewald summation (revisited...

INVALID DOIs

- None
editorialbot commented 1 month ago

Paper file info:

📄 Wordcount for paper.md is 2781

✅ The paper includes a Statement of need section

editorialbot commented 1 month ago

License info:

✅ License found: BSD 3-Clause "New" or "Revised" License (Valid open source OSI approved license)

editorialbot commented 1 month ago

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

srmnitc commented 1 month ago

@gezelter @samwaseda @blackspur 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 with the issues you find 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

srmnitc commented 1 month ago

@samwaseda @blackspur Thanks again for agreeing to review. Just a short reminder, if you need anything from my side to get started, please let me know!

samwaseda commented 1 month ago

Review checklist for @samwaseda

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

samwaseda commented 1 month ago

Since the package contains quite a large number of files and lines, I think it's going to take a few days to check the functionality claimed in the paper.

In the meantime, I have two questions:

crdrisko commented 1 month ago

Hello @samwaseda, a fresh OpenMD clone could be run using the following commands (assuming no dependencies and no installation):

$ mkdir OpenMD && cd OpenMD
$ git clone https://github.com/OpenMD/OpenMD.git openmd
$ mkdir build && cd build
$ cmake ../openmd/.
$ make
$ cd ../openmd/samples/argon
$ set FORCE_PARAM_PATH=../../forceFields
$ ../../../build/bin/openmd 500.omd

This will allow you to run simulations locally on the most basic of systems. Some of the samples do require our dependencies and MPI to run in a reasonable time frame. More information can be found in our INSTALL.md file. Let me know if anything else is unclear or you're still unable to get OpenMD up and running.

gezelter commented 1 month ago

Hi @samwaseda , we've now added a QUICK_START.md to the main directory which provides a step-by-step walk through of how to start and analyze one of the sample simulations.

samwaseda commented 1 month ago

Hello @samwaseda, a fresh OpenMD clone could be run using the following commands (assuming no dependencies and no installation):

$ mkdir OpenMD && cd OpenMD
$ git clone https://github.com/OpenMD/OpenMD.git openmd
$ mkdir build && cd build
$ cmake ../openmd/.
$ make
$ cd ../openmd/samples/argon
$ set FORCE_PARAM_PATH=../../forceFields
$ ../../../build/bin/openmd 500.omd

This will allow you to run simulations locally on the most basic of systems. Some of the samples do require our dependencies and MPI to run in a reasonable time frame. More information can be found in our INSTALL.md file. Let me know if anything else is unclear or you're still unable to get OpenMD up and running.

Thanks for this snippet! This one is very helpful. I guess there's a small typo though: I presume there should be export FORCE_PARAM_PATH=../../forceFields and not set FORCE_PARAM_PATH=../../forceFields right?

samwaseda commented 1 month ago

td;dr: I would like to see a very short README.md in every example with (1) what the example shows and (2) what to expect

Hi @samwaseda , we've now added a QUICK_START.md to the main directory which provides a step-by-step walk through of how to start and analyze one of the sample simulations.

Thanks for this nice file! Yes, this was exactly the kind of document I wanted to see, or actually it's even more detailed than what I had imagined.

However, I have a bit of trouble connecting points in terms of documentation and actual code. Sorry for requesting a lot of things but I'm sure that this helps potential new users like me.

Now together with QUICK_START.md I see two more sources of documentation:

I can see they contain a huge amount of information, but as someone coming from outside, I would take a look at README or QUICK_START and try out the first examples, which is now possible with the new QUICK_START.md, but then when you go to the actual examples, the documentation is either totally missing, or the content varies largely from one example to another. Since one of the criteria for a JOSS paper includes automated tests, I would love to see what these examples do and what to expect for the output, so that users can know easily how to use it and whether they are using it correctly.

I'm not really sure what kind of purpose the website is supposed to fulfil, because the first posts (along with others) I see on the example page are actually not examples and then the examples do not seem to contain the full amount of information so that people can copy and paste the content to run them. If the page was not meant for new users to learn how to use OpenMD, I guess it's fine, but otherwise I think it's important to have a reference to a folder containing the all necessary files for the user to be able to run it.

samwaseda commented 1 month ago

For the installation: Does something speak against including a step to update $PATH? I can imagine a lot of people want a local installation without root access.

Additionally, the list of dependencies should include the versions used for testing.

crdrisko commented 4 weeks ago

Hello @samwaseda, thanks for your feedback so far. Yes, that set-command was a typo, I must have missed that in my conversions from csh to bash. As for the update to $PATH, there's really no reason to update that variable, other than the convenience of not having to carry around the relative or absolute path every time you want to run one of our applications. But the following:

$ openmd spce.omd
$ ~/path-to-openmd/build/bin/openmd spce.omd

are both equally valid. As one final example for this, when I'm developing/testing a new feature for OpenMD, I don't install or set the build path, because those changes may negatively impact our group, who all use the installed (latest version on main) applications. So, I carry around the relative path to my development build until the feature is verified, then I can go back to using the installed version. Hopefully that was clear, are you potentially looking for an explanation for something like that in the documentation?

To your other point, I will get working on the documentation updates for the individual sample directories and the results we expect from them. I'll update the thread when those changes go live.

crdrisko commented 4 weeks ago

@samwaseda, as a follow up to my post this morning, we've made some progress on implementing your suggested changes to documentation in the sample subdirectories. I've included a template README file that all samples should use, and the samples/air, samples/argon, and samples/builders now all follow this format. I'll continue working on it over the coming days, but hopefully this was the direction you were hoping for.

samwaseda commented 4 weeks ago

So, I carry around the relative path to my development build until the feature is verified, then I can go back to using the installed version. Hopefully that was clear, are you potentially looking for an explanation for something like that in the documentation?

Not necessarily. It just feels to me that with conda and other package managers the tendency is that everything works out of the box so I thought it might be confusing when it might not work depending on the example, but obviously whoever has the slightest understanding of bash they would know how to make it work so if you consider it unnecessary I'm also fine with that.

samwaseda commented 4 weeks ago

@samwaseda, as a follow up to my post this morning, we've made some progress on implementing your suggested changes to documentation in the sample subdirectories. I've included a template README file that all samples should use, and the samples/air, samples/argon, and samples/builders now all follow this format. I'll continue working on it over the coming days, but hopefully this was the direction you were hoping for.

I took a look and yes it's exactly the kind of documentation I was hoping for. I also like the length of the documents.

blackspur commented 4 weeks ago

Review checklist for @blackspur

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

gezelter commented 4 weeks ago

For the installation: Does something speak against including a step to update $PATH? I can imagine a lot of people want a local installation without root access.

Hi @samwaseda , we've added $PATH information in INSTALL.md and in QUICK_START.md

Additionally, the list of dependencies should include the versions used for testing.

This has now been added in both INSTALL.md and README.md

samwaseda commented 3 weeks ago

From my side it's almost good to go. There's just "state of the field" missing in the paper, which is one checkbox in the list above. I guess there's no mentioning of other software packages in the current version? I'm not a big MD person but I would say at least LAMMPS should be there.

gezelter commented 3 weeks ago

@editorialbot generate pdf

Hi @samwaseda , we've added sentences to the introduction of the paper which (we think) helps to put OpenMD in context. This has also added references to LAMMPS, CHARMM, Amber, GROMACS, and OpenMM:

"There are a number of high quality molecular dynamics engines that specialize in materials [LAMMPS] or biomolecular [CHARMM; AmberTools] simulations or are models of computational efficiency [GROMACS; OpenMM]. In this paper, we provide background on an open source molecular dynamics engine, OpenMD, which specializes in complex systems and interfaces and was just released into version 3.1."

editorialbot commented 3 weeks ago

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

srmnitc commented 2 weeks ago

@samwaseda thanks a lot for the review efforts, and thanks @gezelter for making the changes as needed

srmnitc commented 2 weeks ago

@blackspur Do you need anything from my side to get the review started? Please feel free to ask if needed.

samwaseda commented 2 weeks ago

"There are a number of high quality molecular dynamics engines that specialize in materials [LAMMPS] or biomolecular [CHARMM; AmberTools] simulations or are models of computational efficiency [GROMACS; OpenMM]. In this paper, we provide background on an open source molecular dynamics engine, OpenMD, which specializes in complex systems and interfaces and was just released into version 3.1."

It’s a good selection of software packages. It just appears to me that this formulation is different from the JOSS-requirement: “how this software compares to other commonly-used packages”. I would expect a phrase that states the difference between OpenMD and these software packages, and, if possible, in which way OpenMD is better at certain tasks. Obviously there’s no need for OpenMD to be generally better; if the paper states a very specific purpose or a very specific group of users, I’m willing to believe it.

By the way I’m on vacation until the end of the month, so I might not react very fast.

crdrisko commented 2 weeks ago

@editorialbot generate pdf

Hello @samwaseda, we've rewritten a bit from the beginning of the Statement of Need section to reflect that the rest of the paper highlights some of the different ways in which our engine is unique (compared to the engines mentioned before), namely RNEMD, LangevinHull, DR-EAM, and DSF.

OpenMD builds on the foundations of the Object-Oriented Parallel Simulation Engine (OOPSE) program [Meineke2005], rewritten in modern C++. It differs from other contemporary molecular dynamics engines in its focus on complex interfaces. A number of features unique to OpenMD facilitate the study of this problem space, including efficient non-equilibrium algorithms, non-periodic simulations, metal polarizability models, and real space electrostatics. The first such feature is Reverse Non-Equilibrium Molecular Dynamics (RNEMD), ...

Hopefully this is what you were looking for, if not, could we get some clarification of what's needed in this section?

editorialbot commented 2 weeks ago

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