[REVIEW]: sumo: Command-line tools for plotting and analysis of ab initio calculations

[whedon]

Submitting author: @utf (Alex Ganose)
Repository: https://github.com/SMTG-UCL/sumo
Version: v1.0.4
Editor: @kyleniemeyer
Reviewer: @jarvist, @cfgoldsmith
Archive: 10.5281/zenodo.1338124

Status

Status badge code:

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

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

@jarvist & @cfgoldsmith, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @kyleniemeyer know.

Review checklist for @jarvist

Conflict of interest

• As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• I confirm that I read and will adhere to the JOSS 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.4)?
• Authorship: Has the submitting author (@utf) made major 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?
• 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 what problems the software is designed to solve 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 (ideally to solve real-world analysis problems).
• Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• Automated 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 what problems the software is designed to solve 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 @cfgoldsmith

Conflict of interest

• As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• I confirm that I read and will adhere to the JOSS 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.4)?
• Authorship: Has the submitting author (@utf) made major 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?
• 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 what problems the software is designed to solve 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 (ideally to solve real-world analysis problems).
• Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• Automated 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 what problems the software is designed to solve and who the target audience is?
• References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

[whedon]

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @jarvist, 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/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:

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

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

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[kyleniemeyer]

👋 @utf @jarvist @cfgoldsmith

For @jarvist & @cfgoldsmith, note that you each have a separate reviewer checklist above. Thanks for agreeing to review!

[utf]

@kyleniemeyer: I've just had a look through the conflict of interest page you linked in the previous issue. Jarvist and David Scanlon (@scanlond), the last author on this paper, were on a paper together back in 2016: https://aip.scitation.org/doi/abs/10.1063/1.4943973

I should also note that the other reviewer I mentioned (@bjmorgan) has also been on a paper with David within the last 4 years.

[utf]

Actually, sorry for the confusion here, I was mistaken, @jarvist and @scanlond haven't actually worked on a paper together.

[kyleniemeyer]

@utf thanks for checking!

[jarvist]

I have however co-authored a few times with @ajjackson back when were were in the same group. If @kyleniemeyer is happy, I'm happy! I certainly do know these people, but I don't think that's any problem in terms of writing a fair and open review of these codes.

[kyleniemeyer]

@jarvist I can see the partial overlap—thanks for being upfront about it. I agree that these connections should not be an issue.

[kyleniemeyer]

Hey @jarvist @cfgoldsmith, just wanted to check in on the status of your reviews.

[cfgoldsmith]

Thanks, Kyle. As we discussed earlier, I am not going to install the software, but will provide feedback on their objectives within the broader computational chemistry community. I don't see where to put that info. Also, since I'm not installing the software, would you prefer that I check the Funtionality boxes or leave them unchecked?

[jarvist]

Successfully installed & am slowly working my way through its functionality. It's quite a broad package with multiple features, which really requires some 'live' electronic structure calculations to throw it against.

[kyleniemeyer]

@cfgoldsmith yes, that's fine. Aside from checking off items, you can give your feedback to @utf here directly. As for the functionality items, you can just check off if there is evidence of them.

[kyleniemeyer]

@jarvist thanks for the update!

[utf]

Hi guys. Just checking if there is any update on the status of the reviews?

Please get in touch if you have any comments for me.

[cfgoldsmith]

Hi. Here is my review:
“sumo currently only supports VASP” this strikes me as rather curious, since VASP is one of the more expensive packages — even for academics. In the spirit of this journal, they should consider expanding their approach to include many of the open-source packages, such as GPAW, Orca, etc.
The authors fail to mention Molden, which blurs the line between command-line and gui, but is old and free and has much of the same functionality (though certainly not all).

The approach and examples is very specifically geared to solid state physics, even if they don’t explicitly say it. They should at least mention it. It would be helpful to elaborate on what future aspects they would consider, especially for the non solid-state community (e.g. what about electron density, molecular orbitals, charge distribution, etc).

Beyond that, the pack seems well developed, the graphics are appropriate and polished, and I think people will find it useful.

[jarvist]

Sumo does many different things - I haven't had time to try them all out yet! Here's a partial review, with some feedback.

1. It wasn't immediately obvious what the code was for + what it did. 'Ab-initio' is a very general term. Making it clear that these tools are for reciprocal-space band structures / computational solid-state physics would help.
   Having used a few of the components, I think perhaps explicitly describing it as a convenience interface to the many dependent python packages / post-processing tools for VASP calculations is most descriptive. Sign-posting which of the many packages are being used 'under the hood' would also be useful,
   i.e. something like
   An extensive framework for generating high-symmetry k-point paths, by interfacing SeeK-path and SPGlib.
   I also think the tutorials in the online documentation should be signposted more clearly in the README, as the /tests /examples folder don't offer much by themselves.
   I noticed several spelling errors / typos in the STDOUT of the tools.

2. The disk space requirements are very heavy. It became apparent upon installing where the name 'sumo' came from, but it might be an idea to alert the user in the README.md.
   I required the following additional packages to be able to install under pip3 on Debian:-
   sudo apt-get install python3-pip python3-numpy
   The lack of python3-numpy was a little cryptic, pip3 just failed at some point. Perhaps it can be added as a check?
   The Debian additions required Required 72 MB (this was on top of the Python3 runtime).
   Installed by pip3, I ended up with 361 MB in .local/lib/python3.5/site-packages/

3. Tests: I'm not really sure what to do with the tests directory.
   Could you provide a 'runtests.py' in the root? Ideally you would add continuous integration. Reading the source of the tests, it's not obvious that they are tests, but more that they are examples where you can check by eye that some sensible results are produced.
   As Sumo does little computation by itself, maybe this is acceptable, but it should be documented and explained. But you could e.g. have a computational pipeline + at least automatically validate that an output file is produced.

Suggested (partial review) actions:-

• Improve README.md
• Spell check sources, particularly the output to the user (for examples, try grep'ing for 'eleemtns' 'possibily' 'indicies:')
• Clean up / document / implement tests

[utf]

@cfgoldsmith

Thank you for your review.

We appreciate that VASP is a little exclusive which is unfortunate, however, it is very widely used in the field (there have been 25,000 papers published using the code in the last 10 years). Crucially VASP comes with a minimal toolchain, which motivated the development of this package.
We have intentionally designed Sumo to separate VASP-specific details from other elements in order to facilitate interfaces for open-source codes. Our team does not have much hands-on experience with the other codes that would most benefit from this, but contributions would be welcome from e.g. Abinit users to add these interfaces. We will add a section to the README which expressly invites this kind of assistance.

From what I can tell, Molden cannot directly load VASP output files and instead requires additional codes to convert the VASP output into a different format. Accordingly, I think Sumo and Molden occupy different niches. In particular, Molden is primarily focused on 3D visualisation which is not something we plan to support.

We agree that Sumo is currently geared towards the solid-state chemistry and physics communities (although the tools will work equally well for any molecular calculations performed in VASP). Sumo will likely remain focussed on the solid-state for the near future so we can make the mention of "solid-state" more prominent in the package descriptions if desired.

We appreciate that you found the package well developed and think others will find it useful. On this basis, are you able to complete the Review Checklist at the top of this page?

[utf]

@jarvist

Thank you for looking at Sumo.

In response to your numbered points:

1. We agree that ab initio is a general term. As noted in our response to @cfgoldsmith, we will update the documentation to make it clear that this package is geared for solid state analysis.

• We already mention all the packages used under the hood in the README but perhaps we could be more specific where each of these is implemented.
• Do you have a specific suggestion as to how we should signpost the tutorials more clearly? The README contains quite a few links to the documentation already.
• Thanks for noticing the typos, we will go make sure to fix those.

2. For the disk space requirements, unfortunately there isn’t much we can do about that as most of this comes from the package dependencies. We will instead add a warning to the “Requirements” section of the readme to highlight this.

3. Regarding tests: we already have a section in the documentation detailing how to run the tests (see https://github.com/SMTG-UCL/sumo#tests). To be clear, these are definitely testing the code base and are not provided as examples of how to run the code. Furthermore, we already have continuous integration enabled using Travis CI (this is indicated by the “build: passing” button at the top of the README). It isn't really expected that general users will poke around the test and example directories, as they are encouraged to install the package from PyPI.

Thank you again for reviewing Sumo.

[cfgoldsmith]

Yes, happily. Well done.

[jarvist]

To be clear - I'm certainly not requesting any extra development in response to my review, just more + clearer signposting in the documentation.

Apologies for missing the CI - would it be possible to add a code-coverage badge? I will investigate the unit tests next week. A two line README.md in the tests directory would not go amiss, repeating the info in the main README.md.

[kyleniemeyer]

@jarvist @cfgoldsmith thanks for your reviews!

@utf please let us know when you've made changes in response to their comments, and specifically what changes you made. (If you open up issues or PRs on your own repository in response, you can link to them here to make it even easier.)

[ajjackson]

I have pushed an update to master that should address these review points. @utf has already assaulted the collection of typos.
Summary of changes pasted here from the commit message:

- Emphasise solid-state scope
- Emphasise use of other Python packages
- Add Contributing guidelines
- Raise links to Tutorial and Gallery
- Rework installation/requirements notes
- Add README to test directory

And "ab initio calculations" in the setup.py (i.e. for PyPI description) is replaced with "ab initio solid-state calculations".

[jarvist]

With these edits, README.rst is much more clear, and I think you have covered all of my partial review points.

Running your unit tests as your documented
python3 -m unittest discover tests
in the pip-installed ~/.local/lib/python3.5/site-packages/sumo/ (maybe point users in this direction in the README?), resulted in:

Ran 97 tests in 3.198s

FAILED (failures=4, errors=14)

(Full STDOUT here: https://gist.github.com/jarvist/ae461f6dd22e1f989a0625bbf4b63f3f )

Most of the errors looked like it trying to find data sets (bz2 / gz) files, which might be hard-coded / not present in the repository?

[ajjackson]

Running from inside ~/.local/lib is rather unorthodox, does it work from a cloned source directory?

The installer isn't set up to copy test files into lib because they aren't part of the library.

[ajjackson]

@jarvist are the tests working for you when they are run from the source folder? In the README we say "the unittests can be run (from the root directory of the project) ..."

The Travis logs show the process followed there, which simply clones the repo https://travis-ci.org/SMTG-UCL/sumo/jobs/402148229

[utf]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[utf]

@jarvist we've updated the paper to clarify in several places, including the paper title, that the the code is specifically for solid-state materials analysis.

Does that look ok now?

[jarvist]

Adding an explicit:

git clone https://github.com/SMTG-UCL/sumo.git
cd sumo

To the quoted shell commands for the developer installation would make it even clearer.

But I'm happy to sign off on this JOSS review as done!

[kyleniemeyer]

@utf only one comment on the paper before we accept: the reference for Togo 2013 is missing some info, like a DOI or URL. Once you've added that and confirmed the paper looks all good, then please archive your entire repo and provide the associated DOI here.

[ajjackson]

In response to @jarvist's README suggestion: Done! Have also added a note to Dev installation that "Regular users can skip this section!" and added explanation of --user flag.

[ajjackson]

Re: Spglib there doesn't seem to be a permanent identifier, but I have added the website https://atztogo.github.io/spglib/. It's available in the waybackmachine, at least!

[ajjackson]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[utf]

@whedon commands

[whedon]

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

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

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

[utf]

@kyleniemeyer We've made the changes you asked for. The repository DOI is 10.5281/zenodo.1338124 (https://doi.org/10.5281/zenodo.1338124).

[kyleniemeyer]

@whedon set 10.5281/zenodo.1338124 as archive

[whedon]

OK. 10.5281/zenodo.1338124 is the archive.

[kyleniemeyer]

OK @arfon this paper is now accepted

[arfon]

@jarvist, @cfgoldsmith - many thanks for your reviews here and to @kyleniemeyer for editing this submission ✨

@utf - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00717 ⚡ 🚀 💥

[whedon]

🎉🎉🎉 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](http://joss.theoj.org/papers/10.21105/joss.00717/status.svg)](https://doi.org/10.21105/joss.00717)

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

This is how it will look in your documentation:

We need your help!

Journal of Open Source Software 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:

• Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
• Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=