search

openjournals / joss-reviews

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

[REVIEW]: physical_validation: A Python package to assess the physical validity of molecular simulation results #3981

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: @ptmerz (Pascal T. Merz) Repository: https://github.com/shirtsgroup/physical_validation Version: v1.0.4 Editor: @rkurchin Reviewer: @tonigi, @geraldjwang, @zadehsey Archive: 10.5281/zenodo.5815657

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@tonigi & @geraldjwang and @zadehsey, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @rkurchin 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 ✨

Review checklist for @tonigi

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @geraldjwang and @zadehsey

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @tonigi, @geraldjwang and @zadehsey it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 2 years ago

PDF failed to compile for issue #3981 with the following error: 

 Can't find any papers to compile :-(
whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=4.90 s (22.3 files/s, 26820.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          69           1628           4534         102794
reStructuredText                 6            182            127            619
XML                              1              1              0            536
Jupyter Notebook                 5              0          20146            408
YAML                            23             32             15            163
Markdown                         3             18              0             64
DOS Batch                        1              8              1             27
make                             1              4              6             10
-------------------------------------------------------------------------------
SUM:                           109           1873          24829         104621
-------------------------------------------------------------------------------

Statistical information for the repository 'd4400660a7747ec0883c7654' was
gathered on 2021/12/07.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Chris Walker                     1            84              0            0.05
MKiesel                          1             5              0            0.00
Matt Thompson                    1            58            391            0.27
Matthew W. Thompson              2             1             95            0.06
Michael Shirts                   4           125             97            0.14
Pascal Merz                    265        126619          24639           91.98
Pascal T. Merz                   1             5              5            0.01
SimonBoothroyd                   4          4755           1473            3.79
Theodore Fobe                    1             8              8            0.01
Wei-Tse Hsu                      2            13             11            0.01
mrshirts                        41          4744           1303            3.68

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Chris Walker                 84          100.0          0.1               11.90
MKiesel                       4           80.0          6.6                0.00
Matt Thompson                57           98.3          6.9               98.25
Michael Shirts               16           12.8         57.8               12.50
Pascal Merz              104703           82.7          6.4                1.31
SimonBoothroyd             4058           85.3         18.6                9.59
Theodore Fobe                 8          100.0         17.2                0.00
Wei-Tse Hsu                  12           92.3          6.4                0.00
mrshirts                     14            0.3        111.8               14.29
rkurchin commented 2 years ago

@whedon generate pdf from branch joss-paper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 2 years ago

PDF failed to compile for issue #3981 with the following error: 

 [WARNING] Citeproc: citation zadehsey not found
Error producing PDF.
! Undefined control sequence.
\hyper@@link ->\let \Hy@reserved@a 
                                   \relax \@ifnextchar [{\hyper@link@ }{\hyp...
l.357 }

Looks like we failed to compile the PDF
rkurchin commented 2 years ago

@whedon generate pdf from branch joss-paper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 2 years ago

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

rkurchin commented 2 years ago

Just so it's clear, @tonigi is doing one review, and @zadehsey and @geraldjwang are doing the other collaboratively.

ptmerz commented 2 years ago

Thank you @rkurchin for leading this, and @tonigi, @zadehsey and @geraldjwang for agreeing to review, I appreciate your time and effort and I am looking forward to your reviews! Please let me know if there is anything I can help with!

tonigi commented 2 years ago

And here comes my "long form" review...

The paper describes the physical_validation package, which performs a number of statistical tests grounded on the statistical mechanics of atomistic simulations. The need for such validation is clear and the package is useful. It does install and run easily, and it has state-of-the-art packaging (conda, pip, docs, tutorials and so on).

I only have minor suggestions on the structure of the paper.

  1. It has a long "statement of need" section, a "prior work" section and... no "body"? It could make sense, for example, to make the second paragraph of the statement of need into a "Functionalities" paragraph, or similar.

  2. As the authors note, the package was already described in the 2018 PLOS ONE paper. A cursory mention of the functionalities added since that release can help frame the current paper better.

  3. GROMACS is mentioned and should be cited.

  4. Regarding the software documentation, arguably a natural use case is to check on user-provided trajectory data. In addition to the GROMACS and LAMPPS parsers, wouldn't it make sense to use MDAnalysis to import trajectory data too? Ability to import data may also be mentioned in the manuscript.

rkurchin commented 2 years ago

🚀 Thanks @tonigi for your prompt and thoughtful review!

Just for reference: What should my paper contain? from JOSS docs

Worth noting that the two included sections are the only ones explicitly required (the general philosophy here is that the software is the primary product being evaluated, and the "paper" is really more there as a companion piece to provide context). That being said, there are no constraints on other allowable sections, and I agree it could be a bit clearer with the slight reorganization suggested.

ptmerz commented 2 years ago

Thank you for your thoughtful review @tonigi!

  1. It has a long "statement of need" section, a "prior work" section and... no "body"? It could make sense, for example, to make the second paragraph of the statement of need into a "Functionalities" paragraph, or similar.

I think that's a great suggestion! I added a "Functionality" header before the second paragraph of the "Statement of need" section, and it does indeed make the paper more clear.

  1. As the authors note, the package was already described in the 2018 PLOS ONE paper. A cursory mention of the functionalities added since that release can help frame the current paper better.

This was the intention we had when writing second paragraph of the "Prior work" section. Do you have suggestions how to improve it?

  1. GROMACS is mentioned and should be cited.

Agreed, I have added a citation.

  1. Regarding the software documentation, arguably a natural use case is to check on user-provided trajectory data. In addition to the GROMACS and LAMPPS parsers, wouldn't it make sense to use MDAnalysis to import trajectory data too? Ability to import data may also be mentioned in the manuscript.

These are great suggestions. I added examples of how to import trajectory data from MDAnalysis at https://physical-validation.readthedocs.io/en/stable/simulation_data.html#use-mdanalysis-to-read-position-and-velocity-trajectory.

We are careful in advertising the ability to import data as an integral part of the package. The parsers are very useful, but we have learned that writing and maintaining such parsers is extremely difficult. The large array of functionality of the molecular simulation packages makes it very hard to cover all (edge) cases, and simulation input and output files are often rather unstable formats which tend to change between versions as functionality is added or removed. We decided to keep shipping the parsers since they are useful to many users and correct to the best of our knowledge. We have, however, focussed the documentation on providing the input to physical_validation by other means (e.g. with the help of Python APIs of the software packages or other libraries such as MDAnalysis), which we think is the more future-proof way to interface with the package.

Again, I really appreciate your time and your thoughtful review!

ptmerz commented 2 years ago

@whedon generate pdf from branch joss-paper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 2 years ago

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

ptmerz commented 2 years ago

Please see above ⬆️ for the most recent version of the paper including @tonigi's suggestions.

tonigi commented 2 years ago

Thanks for the response and the changes. The fact that the new features are under the heading "Prior work" is slightly confusing, but subjective after all. I have no further comments :)

ptmerz commented 2 years ago

I see what you're saying now, thanks for clarifying. Maybe renaming the section to "Relation to prior work" would make it less confusing?

tonigi commented 2 years ago

Sounds a reasonable solution.

ptmerz commented 2 years ago

I have renamed the section. Thank you again for your review, I very much appreciate your suggestions!

ptmerz commented 2 years ago

@whedon generate pdf from branch joss-paper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 2 years ago

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

rkurchin commented 2 years ago

@whedon re-invite @geraldjwang as reviewer

whedon commented 2 years ago

OK, the reviewer has been re-invited.

@geraldjwang please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

rkurchin commented 2 years ago

@whedon re-invite @zadehsey as reviewer

whedon commented 2 years ago

OK, the reviewer has been re-invited.

@zadehsey please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

ptmerz commented 2 years ago

Thank you @geraldjwang and @zadehsey, I appreciate your time reviewing this!

Hi @rkurchin, it seems that all reviewers completed their checklists! What are the next steps?

Nano2Macro commented 2 years ago

I really enjoyed the idea behind the work and believe many would benefit from it. Going over the documentation and example files, I had a number of minor suggestions:

Integrator_validation

Kinetic_energy_distribution

ptmerz commented 2 years ago

Thank you for your comments @zadehsey!

Integrator_validation

  • The cutoff radius of the LJ potential is given as “1 nm”. I believe that it will be more generalizable if the values are non-dimensionalized with respect to the parameters of the interatomic potential. The same applies to the timestep of the simulation as well.

I agree that from a theoretical perspective, using simulation parameters which are depending on interaction or other system parameters makes a lot of sense. In the scope of the physical validation analysis tool, I do however think that it is more useful to have examples that use the type of parameters that users are required to set. In many biomolecular simulation packages, setting absolute values is the norm. See for example GROMACS (parameter dt in ps, parameter rvdw in nm), OpenMM(setStepSize in ps, setCutoffDistance function in nm), LAMMPS (timestep command in time units, lj/cut command sets LJ cutoff in distance units [which is not depending on the LJ parameters except for units lj]). I would therefore prefer to leave the examples in absolute units, if you don't have strong objections!

  • The plot axis needs to be fixed. Potentially using a scientific format. Also, it will be easier to read the plot if a marker is added where the actual data point is. Especially when the behaviour is non-monotonic

Fixing the plot axis is a non-trivial problem. Note that the tool is plotting plots as a visual aid, but is not aiming at producing publication-ready figures. The latter is an almost impossible task given that there is no way to know the values of the integrator convergence before performing the analysis. I fully agree that in the example, it would be nice to have the plots all sharing the same fixed axis. But they come from three independent analysis runs, and I don't see an obvious way how to handle this in general.

I think that having a plot marker is a great thought. I implemented this suggestion in https://github.com/shirtsgroup/physical_validation/pull/201, and the updated documentation looks like this https://physical-validation.readthedocs.io/en/latest/examples/integrator_validation.html. The changes are merged, and I will push a new stable version containing this patch once we conclude this review!

Kinetic_energy_distribution

  • I think that it will be easier to define variables for number of atoms per molecule, degree of constraints, etc. so the user will change those and the description of the variable is also more clear

I changed the notebook to have these in variables. See https://physical-validation.readthedocs.io/en/latest/examples/kinetic_energy_distribution.html.

  • Visually, it may be useful to add a vertical line showing the mean values obtained from the trajectory and the analytical solution

I agree very much with your thought - we actually had that implemented for a while 😃 We found, however, that in any real-world use, the mean values of total kinetic energy estimates are almost always extremely close. (NB: This is actually one of the reason the distribution test is so useful - the mean might be correct, but if the distribution is not followed, the simulation might still be physically invalid!) The vertical lines added more confusion than clarity, so we decided to remove them.

Thank you again for your thoughtful review, I really appreciate your time!

Nano2Macro commented 2 years ago

Thank you for your response @ptmerz. Regarding the first point, I do not have a strong objection and understand why you would want to have the values presented in the current format. However, as the example used is specifically LJ, I believe that at least mentioning the length-scale and energy-scale there as a reference would be helpful for those who prefer to think in non-dimensional terms

ptmerz commented 2 years ago

That is a good point @zadehsey. Adding some information on the LJ parameters used and setting the cutoff in relation to that adds value to the example. I have done that in https://github.com/shirtsgroup/physical_validation/pull/202 and it is now reflected in the documentation on https://physical-validation.readthedocs.io/en/latest/examples/integrator_validation.html.

Thanks again for your suggestion!

geraldjwang commented 2 years ago

@ptmerz and team: I wanted to, above all else, congratulate you on a thoughtful (and delightful (and sorely needed)) package, which has really been an end-of-year treat to look through! (Seriously!) I appreciate the thoughtful iterations w.r.t. points raised by @zadehsey and @tonigi. Our community needs this kind of work. As far as the formal review process, I have nothing further to request: Everything on the checklist is clearly there, and this is overall a wonderful (and, again, sorely needed) step in the direction of more automated and more physics-grounded verification of MD results.

I have several ideas for additional tests that could be quite informative to develop within this framework, but I'll keep that discussion separate from the review process since these ideas are immaterial to the suitability of this work as is (this work is, in my view, wonderfully suitable for JOSS in its current form).

I also wish to add (although this is also immaterial to the formal review) that I will be absolutely delighted to point students towards this package in my molecular simulation course when I am teaching it again next year. (To be clear, they will still have to verify that, e.g., their simulation results satisfy Equipartition Theorem via their own elbow grease, but it's wonderful to be able to then show them that tools such as yours exist, and are actively being developed by thoughtful people!)

rkurchin commented 2 years ago

Thanks everyone! @ptmerz, I'll do an editorial pass over the manuscript and send any comments shortly. In the meantime, the next steps for you are:

  1. Merge any and all changes from this review into your main branch (if you haven't already) and issue a new version tag. (If you want to merge in the paper itself, you may, but it is not required that the actual manuscript live into the repo in perpetuity since JOSS will host it and you can simply add a badge link or whatever you like. But the actual changes to software and docs do need to be merged!)
  2. Create a DOI for the contents of the repo at the same commit corresponding to that version tag, e.g. using figshare or Zenodo. Please make sure that the metadata (version number, title, author list, etc.) match those of your manuscript.
  3. Post a comment here with the version number and DOI.
rkurchin commented 2 years ago

@whedon generate pdf from branch joss-paper

whedon commented 2 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 2 years ago

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

rkurchin commented 2 years ago

Minor editorial suggestions:

ptmerz commented 2 years ago

Thank you for your time and your thoughtful review @geraldjwang. It am very happy to hear that you find our work useful - reading your comment was indeed a nice way to start the new year :)

As for your suggestions for additional tests, please do not hesitate to get in touch either via the GitHub issue tracker, or directly via e-mail. We would love to add new functionality that is useful to the community, so we definitely welcome ideas and / or contributions.

ptmerz commented 2 years ago

Thank you for your editorial suggestions @rkurchin, they are much appreciated!

All of the reviewers' suggestions have been merged. I have also included your editorial suggestions in the manuscript, and merged the manuscript into the main branch.

I created a new version including all the latest changes:

ptmerz commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

rkurchin commented 2 years ago

@whedon set v1.0.4 as version

whedon commented 2 years ago

OK. v1.0.4 is the version.

rkurchin commented 2 years ago

@whedon set 10.5281/zenodo.5815657 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5815657 is the archive.

rkurchin commented 2 years ago

@whedon check references

whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1371/journal.pone.0202764 is OK
- 10.1021/ct300688p is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.softx.2015.06.001 is INVALID because of 'https://doi.org/' prefix