Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: pylj: A teaching tool for classical atomistic simulation #19

Closed
whedon opened this Issue Jun 26, 2018 · 64 comments

Comments

Projects
None yet
5 participants
@whedon
Copy link
Collaborator

whedon commented Jun 26, 2018

Submitting author: @arm61 (Andrew McCluskey)
Repository: https://github.com/arm61/pylj
Version: v1.0.0
Editor: @labarba
Reviewer: @shivupa, @TJFord
Archive: 10.5281/zenodo.1312617

Status

status

Status badge code:

HTML: <a href="http://jose.theoj.org/papers/58daa1a1a564dc8e0f99ffcdae20eb1d"><img src="http://jose.theoj.org/papers/58daa1a1a564dc8e0f99ffcdae20eb1d/status.svg"></a>
Markdown: [![status](http://jose.theoj.org/papers/58daa1a1a564dc8e0f99ffcdae20eb1d/status.svg)](http://jose.theoj.org/papers/58daa1a1a564dc8e0f99ffcdae20eb1d)

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) 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

@shivupa & @TJFord, 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/jose-reviews/invitations

The reviewer guidelines are available here: https://jose.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @labarba know.

Review checklist for @shivupa

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v1.0.0)?
  • Authorship: Has the submitting author (@arm61) made substantial contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
  • Example usage: Do the authors include examples of how to use the software?
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @TJFord

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v1.0.0)?
  • Authorship: Has the submitting author (@arm61) made substantial contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
  • Example usage: Do the authors include examples of how to use the software?
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jun 26, 2018

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @shivupa, it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐️ Important ⭐️

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/jose-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/jose-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
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jun 26, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jun 26, 2018

@shivupa

This comment has been minimized.

Copy link
Collaborator

shivupa commented Jun 26, 2018

@whedon commands

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jun 26, 2018

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jun 26, 2018

Isn't our bot whedon just the cutest thing?

@TJFord

This comment has been minimized.

Copy link
Collaborator

TJFord commented Jun 26, 2018

A few comments:

  1. I really like the idea of running a demo through Jupyter, as it is convenient to test the code directly. I tried the cloud version for molecular_dynamics but not successful. Please correct me if I did it wrong. http://35.230.133.1/notebook/notebooks/examples/molecular_dynamics.ipynb#
    The error message is here:
TypeError                                 Traceback (most recent call last)
 in ()
----> 1 system = md_simulation(100, 273.15, 100, 5000, 50)
 in md_simulation(number_of_particles, temperature, box_length, number_of_steps, sample_frequency)
     16                                                                                                  system.cut_off)
     17         # Run the equations of motion integrator algorithm
---> 18         system.particles = md.velocity_verlet(system.particles, system.timestep_length, system.box_length)
     19         # Sample the thermodynamic and structural parameters of the system
     20         system = md.sample(system.particles, system.box_length, system.initial_particles, system)
TypeError: velocity_verlet() missing 1 required positional argument: 'cut_off' 
  1. I tried to run it on a linux computer through a terminal, it was not successful either. I followed exactly all the steps shown in README.md. I suspected I missed installing some libraries. I think it will be beneficial if a detailed installation manual can be provided.
@shivupa

This comment has been minimized.

Copy link
Collaborator

shivupa commented Jun 26, 2018

Can you open this as an issue on the submission repo. I had some questions about this too.

@TJFord

This comment has been minimized.

Copy link
Collaborator

TJFord commented Jun 26, 2018

@shivupa I just opened an issue at arm61/pylj#11

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jun 27, 2018

@shivupa, @TJFord -- the submitting author is also working on improved narrated examples for this package. We will wait for a heads-up that those are ready, and then please have a look at them, too.

@TJFord

This comment has been minimized.

Copy link
Collaborator

TJFord commented Jun 30, 2018

@labarba @shivupa it looks like the example monte carlo simulation is not working either. @arm61, can you double check it?
Also a few other minor suggestions:

  1. would it be better if the title is changed to "pylj: A teaching tool for classical molecular dynamics simulation "? as the focus of the tool is on molecular dynamics simulation.
  2. I would like to cite a few other widely used open source molecular dynamics simulation packages in the paper, such as lammps, gromacs, etc., for those students if they want to know more about it.
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
 in ()
----> 1 system = mc_simulation(2, 273.15, 45, 100000, 100)

 in mc_simulation(number_of_particles, temperature, box_length, number_of_steps, sample_frequency)
      8     system.particles, system.distances, system.energies = comp.compute_energy(system.particles, 
      9                                                                               system.box_length,
---> 10                                                                               system.cut_off)
     11     old_energy = system.energies.sum()
     12     system = mc.sample(system.particles, system.box_length, old_energy, system)

AttributeError: 'System' object has no attribute 'cut_off'
@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jun 30, 2018

I agree that it should say "molecular simulation," or better "molecular dynamics simulation," everywhere.

Folks from outside the MD word will have no idea what the author means otherwise.

@shivupa

This comment has been minimized.

Copy link
Collaborator

shivupa commented Jul 1, 2018

I'd suggest "molecular simulation" since this encompasses molecular dynamics and Monte Carlo.

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 2, 2018

Can we possibly agree on "classical atomistic simulation"? For the following reasons:

  • Since we are only really dealing with individual atoms of argon in pylj it is a bit false to say molecular.
  • I would like to keep "classical" in there as pylj only performs classical potential based simulation, e.g. there are not quantum mechanical methods (e.g. DTF) to be seen.

@labarba labarba changed the title [REVIEW]: pylj: A teaching tool for classical simulation [REVIEW]: pylj: A teaching tool for classical atomistic simulation Jul 2, 2018

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 3, 2018

Paper title has been updated in arm61/pylj@bb492be

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 5, 2018

There are now two new examples of possible exercises showing the utility of pylj these can be found https://github.com/arm61/pylj/tree/master/examples . This means there is now the molecular dynamic and monte carlo examples showing how pylj can be used for teaching about simulation methods and the ideal gas example showing an application of pylj to build on traditional physical chemistry lecture material.

Note that to run these you need to build the most recent commit of pylj (this is because in developing the tutorials I recognised some more useful pedagogical methods that could be applied). This has also lead to the pairwise module which is a pure python implementation of the comp module (this is MUCH slower than the cython but possibly more pedagogically interesting).

I hope this takes the submission up to the expected level that is required. Sorry about the delay (life gets in the way sometimes).

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 6, 2018

I have an observation that is optional for you to address, but I ask you to consider… You are using British spelling in the mc.initialise() function. This is likely to trip many if not most users! I see that you are based in Bath, so it make sense for your local users. Maybe you can overload the name and have two functions that do the same thing?

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 6, 2018

This is a fair comment (especially considering the python preference for US english). I have mapped the functions in arm61/pylj@2697140

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 6, 2018

@TJFord I have added references to the paper to gromacs, lammps and dlpoly in arm61/pylj@21baa24

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 6, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 6, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 6, 2018

@TJFord

This comment has been minimized.

Copy link
Collaborator

TJFord commented Jul 6, 2018

@labarba I think the author @arm61 has addressed all my concerns. I suggest that it is ready for publication.

@shivupa

This comment has been minimized.

Copy link
Collaborator

shivupa commented Jul 8, 2018

@labarba I also agree @arm61 has addressed all concerns and it is ready for publication.

@shivupa

This comment has been minimized.

Copy link
Collaborator

shivupa commented Jul 8, 2018

@TJFord I'm unsure how whedon works, but we may need to check all the boxes above to proceed.

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 11, 2018

@labarba it seems that both reviewers are happy. I was wondering what the next stage was?

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 15, 2018

Okay, I think that is everything that you've mentioned resolved. Hopefully, the paper is more suitable not (commits arm61/pylj@8d6ebd3 arm61/pylj@94a5a49 arm61/pylj@ddf306f arm61/pylj@22b926f). The only thing that you suggested that I haven't done is captalising argon, this is because (as I understand) it is IUPAC convention to not treat element names as proper nouns (https://chemistry.stackexchange.com/questions/6381/why-do-people-often-capitalize-element-names).

I have also added a mention of the examples in the README (commit arm61/pylj@6669515). I don't go into detail as my plan is to mention the paper once it is published which gives more details.

I have move the "How to teach with pylj" bit from the guide.ipynb file to the "Usage" section of the paper and as a result have decided that the guide.ipynb file is surplus to requirements.

Thanks for the thorough comments @labarba

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 15, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 15, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 15, 2018

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

Good changes!

Little fixes:

  • you have punctuation before citations that refer to previous passage; better after
  • string of citation for other packages: better to interleave, "Gromacs (ref), LAMMPS (ref), DLPOLY (ref)"
  • typo: repoository -->repository
  • typo: how to use pylj to simulated --> simulate
  • typo: using eaither --> either
  • run-on sentence: "…in a teaching laboratory setting, with this in mind we suggest" --> period or semi-colon after "setting" and comma after "mind"
  • typo (p.2): folloiwng --> following
  • period after "courses"
@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 15, 2018

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 15, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 15, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 15, 2018

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

OK! Now, please make a deposit on Zenodo or your favorite archive (or update a versioned DOI, if the case), and give me the DOI here.

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 15, 2018

The DOI is 10.5281/zenodo.1312617

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

@whedon set 10.5281/zenodo.1312617 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 15, 2018

OK. 10.5281/zenodo.1312617 is the archive.

@labarba labarba added the accepted label Jul 15, 2018

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

I need to do a little local processing to publish your paper, but will have to do that tomorrow, due to commitments now.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

Something is wrong with the Zenodo DOI you gave me
https://doi.org/10.5281/zenodo.1312617

"This DOI cannot be found in the DOI System"

Can you double check?

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 15, 2018

that should be correct zenodo link. It might be the case that it takes an hour or two for the DOI that zenodo provides to mature? I created a new release so the DOI is very "fresh"

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

OK. We'll wait. If the DOI does not resolve tomorrow morning your time (please check), then send an email to Zenodo help. I won't publish the JOSE paper until the DOI of the archive resolves.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 15, 2018

I mean, at the moment it is pushed to JOSE-papers, but I haven't made a Crossref deposit.

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 16, 2018

I checked and the DOI still gives me an error: https://doi.org/10.5281/zenodo.1312617

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 16, 2018

Same, I have contacted Zenodo via both twitter and email to see what the issue is

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 16, 2018

It is resolved now

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 16, 2018

Crossref threw citation errors. Please fix DOI links for the Gromacs paper and Plimpton 1995.

I will have to do the whole process again!

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 16, 2018

Resolved in arm61/pylj@ce91689

Sorry about that, apparently the bibtex served by sciencedirect incorrectly prepends "http://doi.org/" for the doi...

@labarba labarba closed this Jul 16, 2018

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Jul 16, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://jose.theoj.org/papers/10.21105/jose.00019/status.svg)](https://doi.org/10.21105/jose.00019)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/jose.00019">
  <img src="https://jose.theoj.org/papers/10.21105/jose.00019/status.svg" alt="DOI badge" >
</a>

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Education is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Jul 16, 2018

@arm61 -- your paper is now published in JOSE ‼️ Congratulations 🎉
You can now add the happy green JOSE button on your repository.

@shivupa, @TJFord — Thank you again for your review 🙏

@arm61

This comment has been minimized.

Copy link

arm61 commented Jul 16, 2018

Yay! Thanks @labarba for your awesome editorial work, and thanks again to @shivupa and @TJFord!

@shivupa

This comment has been minimized.

Copy link
Collaborator

shivupa commented Jul 16, 2018

Congrats! I'm happy to have been a part of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.