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]: MDCraft: A Python assistant for performing and analyzing molecular dynamics simulations of soft matter systems #7013

Closed editorialbot closed 3 weeks ago

editorialbot commented 1 month ago

Submitting author: !--author-handle-->@pw0908<!--end-author-handle-- (Pierre Walker) Repository: https://github.com/bbye98/mdcraft Branch with paper.md (empty if default branch): main Version: v1.2.0 Editor: !--editor-->@srmnitc<!--end-editor-- Reviewers: @raynol-dsouza, @aazocar Archive: 10.5281/zenodo.13308642

Status

status

Status badge code:

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

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

@raynol-dsouza & @aazocar, 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 @aazocar

📝 Checklist for @raynol-dsouza

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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/1.457481 is OK
- 10.1063/1.479595 is OK
- 10.1063/1.3216473 is OK
- 10.1021/acs.macromol.0c02001 is OK
- 10.1371/journal.pcbi.1005659 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1002/jcc.21787 is OK
- 10.1016/j.bpj.2015.08.015 is OK
- 10.1021/acs.macromol.3c02437 is OK
- 10.1021/acs.langmuir.3c03640 is OK
- 10.21203/rs.3.rs-3643582/v1 is OK
- 10.48550/ARXIV.2403.08148 is OK
- 10.1038/s41592-019-0686-2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 month ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.13 s (829.5 files/s, 191820.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          54           4293           8453           7683
C++                              8            139             52           1020
CUDA                             5            199             63           1006
Markdown                         6            136              0            366
C/C++ Header                    10            107            345            300
TeX                              1             13              0            210
Jupyter Notebook                 3              0            386            177
YAML                             5             12             21            135
CMake                            3             29             22             91
SWIG                             1             19             11             90
TOML                             1              2              0             36
SVG                              4              0              0             24
reStructuredText                 7             51            142             20
CSS                              1              0              0              8
JSON                             1              0              0              4
INI                              1              0              0              3
-------------------------------------------------------------------------------
SUM:                           111           5000           9495          11173
-------------------------------------------------------------------------------

Commit count by author:

    43  bbye98
     5  pw0908
     2  Benjamin Ye
editorialbot commented 1 month ago

Paper file info:

📄 Wordcount for paper.md is 767

✅ The paper includes a Statement of need section

editorialbot commented 1 month ago

License info:

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

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

@pw0908 @raynol-dsouza @aazocar 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 just judging 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 the reviews

aazocar commented 1 month ago

Review checklist for @aazocar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

srmnitc commented 1 month ago

@pw0908 The CI pipeline seems to be failing at the moment with ModuleNotFoundError: No module named 'dynasor'. Could you please take a look and fix this to ensure that the reviewers can install the package?

raynol-dsouza commented 1 month ago

Review checklist for @raynol-dsouza

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

pw0908 commented 1 month ago

@srmnitc The current release under review (v1.1.0) is still fine. This issue appears to be upstream as, even re-running a previous commit which had passed is now failing. We'll contact the dynasor developers, but, this error is related to one of the tests and not the main functionalities of MDCraft.

bbye98 commented 1 month ago

@srmnitc This issue surfaced with my latest push, despite there being no changes to any CI workflow files, dependency files, or unit tests. I will try to resolve this ASAP, but cloning the repository and following the exact steps in the CI workflow yielded no issues!

bbye98 commented 1 month ago

@srmnitc I was able to find the dependency issue for the unit tests and have resolved it at https://github.com/bbye98/mdcraft/issues/3#issuecomment-2240591324.

bbye98 commented 1 month ago

@editorialbot generate pdf

I've updated the paper bibliography for a paper authored by me that recently got published. The only change is replacing the arXiv DOI with the J. Chem. Theory Comput. DOI.

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

@editorialbot generate pdf

I've updated the paper bibliography for a paper authored by me that recently got published. The only change is replacing the arXiv DOI with the J. Chem. Theory Comput. DOI.

Thank you for the change!

pw0908 commented 1 month ago

@aazocar @raynol-dsouza Please let us know if you have any questions / run into any issues! @bbye98 and I are at the ready to assist you both!

raynol-dsouza commented 1 month ago

@pw0908 @bbye98 I ran into a (maybe rather trivial) issue that I have raised here.

raynol-dsouza commented 1 month ago

Dear all,

I find that the code for this package is well written and documented, with clear instructions for its installation. The tests seem comprehensive enough to offer thorough coverage of the core functionality. There is no doubt a substantial scholarly effort here, as evident from the source code.

My concern, however, is justifying the "extension to existing functionality" (under the checkpoint Functionality) from just a single example. The accompanying paper states numerous use cases for the package. I believe it is necessary to have at least a couple of more examples in well documented notebooks (for example, using the package to run and post-process a lammps simulation).

@srmnitc if this is not necessary, I will continue with my checklist.

bbye98 commented 1 month ago

@pw0908 @bbye98 I ran into a (maybe rather trivial) issue that I have raised here.

I should have resolved this issue with this push. Please let me know!

bbye98 commented 1 month ago

@raynol-dsouza A number of the extensions we mention are used in our most recent paper, for which the source code is available here.

The OpenMM and LAMMPS input scripts in /benchmark use our unit reduction strategy in OpenMM, our custom GCMe pair potential in OpenMM, our implementation of the slab correction in OpenMM, and our implementation of the image charges in both OpenMM and LAMMPS.

The Jupyter Notebook analysis_gcme.ipynb showcases some of the analysis modules through the evaluation of the number density/charge density/electrostatic potential density and radial distribution function. I also have a radius of gyration example that I am happy to add.

We do plan on adding (and have already started working on) a user guide in the near future showing the entire process from setting up the simulation to analyzing the data (essentially just piecing together nvt_polyanion_counterion_solvent.py and analysis_gcme.ipynb). However, as I have my Ph.D. defense in less than two weeks, I do not currently have the bandwidth to finish drafting the user guide.

pw0908 commented 1 month ago

@bbye98 @raynol-dsouza I'll handle porting over the code to mdcraft. Alternatively, we could just explicitly reference the repository on the MDCraft website? Would that be sufficient @raynol-dsouza? The only thing is that those examples might be too dense; splitting them up will make it easier to follow.

raynol-dsouza commented 1 month ago

I should have resolved this issue with https://github.com/bbye98/mdcraft/issues/4#issuecomment-2256606292. Please let me know!

@bbye98 It works now! 👍

raynol-dsouza commented 1 month ago

A number of the extensions we mention are used in our most recent paper, for which the source code is available here.

@bbye98 This is really, really nice!

While I personally would prefer having minimal working examples of these in an /examples directory, considering @bbye98's upcoming thesis defense and @pw0908's suggestion to explicitly reference the gcme repository, I defer to the better judgment of @srmnitc.

I am happy as long as there is a clear demonstration of a fair amount of functionality mentioned in the write-up!

bbye98 commented 1 month ago

@raynol-dsouza @pw0908

As a sign of good faith and to preview the completed guide, I have pushed the work-in-progress user guide for the GCMe project: https://mdcraft.readthedocs.io/en/latest/notebooks/user_guide/gcme_polyelectrolyte_edlc.html

When completed, it will have a full, working OpenMM input script and multiple analysis examples at the end.

aazocar commented 1 month ago

@bbye98 @pw0908 @srmnitc MDCraft is a very well "crafted" package :) There is good API documentation, robust testing and I was able to install and run the given example. I can also see there is considerable scholarly effort and added value. I have now finished my initial review and I have opened some issues on the repo. Closing these issues would allow me to check the rest of the items in my review checklist. In general, I would like to see more examples and support for the functionality claims stated on the paper. I also support the issue raised by @raynol-dsouza and having a User Guide, even if on the shorter side, will address most of my suggestions. I do not have extensive experience on soft matter systems, but for an interested potential user, like me, more examples would be a great help.

srmnitc commented 1 month ago

@raynol-dsouza and @aazocar thank you for your extensive reviews, and @bbye98 and @pw0908, thanks for getting started on the issues.

After going through the issues, a common point that both reviewers have pointed out is the lack of examples that can help validate the functional claims of the software. The new example is definitely helpful, so is adding a link to the other repository where more examples can be found. However, the solution here is to add more examples to the user guide section. These need not be fully fledged use cases. For example, using custom force fields with OpenMM is mentioned as a feature of the paper. Some small examples which illustrate how one can switch to a custom force field that MDCraft provides would indeed be extremely useful. In general, my suggestion would be to also envision a new user who might not really be interested in running the full workflow as you have published before, but rather wants to use one of the modular features that MDCraft provides.

Thanks again for all the work from both the authors and reviewers!

aazocar commented 1 month ago

@editorialbot generate pdf

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:

aazocar commented 1 month ago

All the issues from my review have been closed now and I have completed all points from the review checklist. Thanks to @pw0908 and @bbye98 for addressing all aspects and making the changes.

A recommendation for the future would be to increase the examples on the User Guide, not necessarily in very long or dense examples, but rather small blocks that showcase specific functions. In my experience, the support of new and less experienced users helps to increase the re-usability of the software.

Another small suggestion would be to include a CITATION.cff file in the repository to clearly state how others should cite the software.

Again, these are small recommendations and not blocking acceptance. Therefore, I recommend the publication of MDCraft in JOSS.

Thanks for the opportunity to review this software.

raynol-dsouza commented 1 month ago

@srmnitc @pw0908 @bbye98

After the changes made following the issues raised by @aazocar and myself, all the points on my checklist have also been completed. I also recommend the publication MDCraft in JOSS. :)

I have no additional suggestions beyond what @aazocar has already recommended.

Cheers!

bbye98 commented 1 month ago

@aazocar @raynol-dsouza Thank you both so much for your time. I will definitely work to incorporate your recommendations and suggestions, and I will update the User Guide section in the upcoming month with a number of examples to further highlight the various functionalities of MDCraft. This is a project near and dear to my heart, so I expect to continue developing, contributing to, and maintaining for a long time. I am glad to hear that you both found the package useful.

pw0908 commented 1 month ago

@srmnitc Is there anything you'd like us to modify/add? I realise we also need to make the zenodo for MDCraft so we'll try to get that done by today.

srmnitc commented 1 month ago

@editorialbot generate pdf

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

@editorialbot check references

editorialbot commented 1 month ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/1.3216473 is OK
- 10.1021/acs.macromol.0c02001 is OK
- 10.1063/1.479595 is OK
- 10.1063/1.457481 is OK
- 10.1371/journal.pcbi.1005659 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1002/jcc.21787 is OK
- 10.1021/acs.langmuir.3c03640 is OK
- 10.1021/acs.macromol.3c02437 is OK
- 10.1016/j.bpj.2015.08.015 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1021/acs.jctc.4c00603 is OK
- 10.21203/rs.3.rs-3643582/v1 is OK
- 10.1016/j.softx.2015.06.001 is OK
- 10.1021/ct400341p is OK
- 10.1002/jcc.10243 is OK
- 10.1080/14786436508211931 is OK
- 10.1103/PhysRev.156.685 is OK

MISSING DOIs

- No DOI given, and none found for title: Polymer Physics

INVALID DOIs

- https://doi.org/10.1016/j.cpc.2020.107275 is INVALID because of 'https://doi.org/' prefix
srmnitc commented 1 month ago

@aazocar and @raynol-dsouza Once again, thanks a lot for taking the time to review the packages and for providing detailed feedback. Please consider signing up at https://reviewers.joss.theoj.org/join as reviewers if you have not done already :) Thank you for your efforts!

@bbye98 @pw0908 Thanks for implementing the changes. I have opened an issue and a PR, after addressing this we can move on with the rest of steps.

srmnitc commented 3 weeks ago

@editorialbot generate pdf

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 3 weeks ago

@editorialbot check references

editorialbot commented 3 weeks ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/1.3216473 is OK
- 10.1021/acs.macromol.0c02001 is OK
- 10.1063/1.479595 is OK
- 10.1063/1.457481 is OK
- 10.1371/journal.pcbi.1005659 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1002/jcc.21787 is OK
- 10.1021/acs.langmuir.3c03640 is OK
- 10.1021/acs.macromol.3c02437 is OK
- 10.1016/j.bpj.2015.08.015 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1021/acs.jctc.4c00603 is OK
- 10.21203/rs.3.rs-3643582/v1 is OK
- 10.1016/j.softx.2015.06.001 is OK
- 10.1093/oso/9780198520597.001.0001 is OK
- 10.1016/j.cpc.2020.107275 is OK
- 10.1021/ct400341p is OK
- 10.1002/jcc.10243 is OK
- 10.1080/14786436508211931 is OK
- 10.1103/PhysRev.156.685 is OK

MISSING DOIs

- None

INVALID DOIs

- None
srmnitc commented 3 weeks ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

srmnitc commented 3 weeks ago

@pw0908 Could you please do the following tasks:

pw0908 commented 3 weeks ago

@srmnitc I can confirm the authors, affiliations and ORCIDs are all correct. I have also created the automated .zenodo.json file with the same information as the JOSS paper. @bbye98 just passed his defense last friday; once I can get a hold of him again, I'll have him make the release and zenodo repos.

pw0908 commented 3 weeks ago

That was a bit of a struggle. But here are the version number and zenodo DOI:

srmnitc commented 3 weeks ago

@editorialbot set 10.5281/zenodo.13308642 as archive

editorialbot commented 3 weeks ago

Done! archive is now 10.5281/zenodo.13308642

srmnitc commented 3 weeks ago

@editorialbot set 1.2.0 as version

editorialbot commented 3 weeks ago

Done! version is now 1.2.0