search

openjournals / jose-reviews

Reviews for the Journal of Open Source Education (JOSE)
http://jose.theoj.org
Creative Commons Zero v1.0 Universal
33 stars 4 forks source link

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

Closed whedon closed 6 years ago

whedon commented 6 years ago

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

Functionality

Documentation

Software paper

Review checklist for @TJFord

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 6 years ago

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 :tada:.

:star: Important :star:

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 commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

shivupa commented 6 years ago

@whedon commands

whedon commented 6 years ago

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 commented 6 years ago

Isn't our bot whedon just the cutest thing?

TJFord commented 6 years ago

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 commented 6 years ago

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

TJFord commented 6 years ago

@shivupa I just opened an issue at https://github.com/arm61/pylj/issues/11

labarba commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

arm61 commented 6 years ago

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

arm61 commented 6 years ago

Paper title has been updated in https://github.com/arm61/pylj/commit/bb492bed8d8d37656e2534574f8ba4a876d001a6

arm61 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

This is a fair comment (especially considering the python preference for US english). I have mapped the functions in https://github.com/arm61/pylj/commit/26971404a9ccb195d96c3bb75243eb0815b9d7ca

arm61 commented 6 years ago

@TJFord I have added references to the paper to gromacs, lammps and dlpoly in https://github.com/arm61/pylj/commit/21baa244c887c5ced60d227ecd0bc30967ac104e

arm61 commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

TJFord commented 6 years ago

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

shivupa commented 6 years ago

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

shivupa commented 6 years ago

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

arm61 commented 6 years ago

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

arm61 commented 6 years ago

also a massive thanks to @shivupa and and @TJFord for all their hard work on making pylj a MUCH more useful piece of software than previously. Great peer reviewing folks :smile:

labarba commented 6 years ago

Please stay tuned. I am at a conference now. I have to go over the comments/improvements and paper myself before accepting the submission.

labarba commented 6 years ago

@arm61 Could you try moving the positioning of your figure down, so that more text moves to the first page, under Summary? There's a big chunk of white space there.

labarba commented 6 years ago

Editorial feedback

Summary: (l.1) classical simulation—> classical atomistic simulation (l.2) the citation should appear as Author, Year (here and elsewhere) (l.4) define NVE and NVT (l.5) capitalize Argon (l.7) capitalize Matplotlib (l.9) explaination—>explanation (typo) This whole passage is hard to parse. The sentence construction is perhaps a bit ritzy. Can you simplify? Something like: PyLJ is written in Python (using Cython for pairwise interactions) and uses Jupyter notebooks and Matplotlib for visualization (example below). It can be easily deployed in a computer laboratory, and students interact with it without needing to use the command line, as they would when using other molecular dynamics software like Gromacs, DL_poly or others. (Refs.) We provide example notebooks in the repository, showing how to use PyLJ to simulate a 2D gas system using molecular dynamics and Monte-Carlo methods. A variety of other applications are possible. (p.2) “thermostating” is jargon: please define or rephrase

Statement of need: classical simulation—> classical atomistic simulation PyLJ allows this by offering—> PyLJ offers capitalize Matplotlib “In addition to use in the introduction of the simulation itself” —> huh? Please rephrase. —. Audience: you mention in the first sentence undergraduates in chemistry and physics. Can you say more about where in the curriculum this would be used? (what course? what year?)

labarba commented 6 years ago

Hmm... I don't get why your first reference is displayed with the full title in the Summary. The citation looks correct in the paper.md file.

labarba commented 6 years ago

Sorry to give you comments piecemeal… I did the first part on a flight yesterday.

You give a short description of the examples, with a link to the folder in the repo, within the “Statement of Need” section. It doesn’t quite belong there. You could use a new heading “Usage” and talk about the examples there. It would also be good to add a bulleted list of the examples in the README. Navigating down the folders to find the examples is not user-friendly, and since some folders have two notebooks, a user may not know where to start. I, myself, click through the various notebooks, and don’t know what order one is supposed to go through this.

What I’d like to see is a paragraph in the paper (and maybe in a new README in the examples folder) that orients an independent learner or instructor on getting started and adopting this tool. It took me many visits to your repository to finally stumble on the guide.ipynb file just now! Please add a link to this file from the README. I think the “How to teach with PyLJ” section here would be useful to have in the paper. (But please don’t use the un-semantic hyperlinked “here.”)

labarba commented 6 years ago

I went to the Getting Started page on the documentation, and clicked on the Tutorial links, which launch a Binder session. However, they all give a "cannot resolve ref" error.

arm61 commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

labarba commented 6 years ago

Maybe the problem with the citation to LENNARD-JONES is that the entry has no author field.

arm61 commented 6 years ago

Okay, I think that is everything that you've mentioned resolved. Hopefully, the paper is more suitable not (commits https://github.com/arm61/pylj/commit/8d6ebd3a02049f2290249648e4a988ac7d82ad0e https://github.com/arm61/pylj/commit/94a5a49d57cb76e94508bb40b2ee3300e5b03ed1 https://github.com/arm61/pylj/commit/ddf306f3896bc02d6b18555bfa01c1ee0844daa4 https://github.com/arm61/pylj/commit/22b926f1ecfc26b95269cdefd98fe3e427dafdb2). 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 https://github.com/arm61/pylj/commit/666951580418427cc863f7028a39547b5794e8be). 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 commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

labarba commented 6 years ago

Good changes!

Little fixes:

arm61 commented 6 years ago

Done! https://github.com/arm61/pylj/commit/d5f0a23e03c0eb06657db96a9f7c299a4a932902

arm61 commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

labarba commented 6 years ago

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 commented 6 years ago

The DOI is 10.5281/zenodo.1312617

labarba commented 6 years ago

@whedon set 10.5281/zenodo.1312617 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1312617 is the archive.