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]: SurfinPy 2.0: A Phase Diagram Generator for Surfaces and Bulk Phases #4014

Closed
40 tasks done
whedon opened this issue Dec 20, 2021 · 69 comments
Closed
40 tasks done
Assignees
Labels
accepted published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review TeX

Comments

@whedon
Copy link

whedon commented Dec 20, 2021

Submitting author: @jstse (Joshua Tse)
Repository: https://github.com/symmy596/SurfinPy
Branch with paper.md (empty if default branch):
Version: v2.0.2
Editor: @lucydot
Reviewers: @awvwgk, @dandavies99
Archive: 10.5281/zenodo.6250513

⚠️ JOSS reduced service mode ⚠️

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/3e3e853bf4eb3ea9e754228343ad6c8b"><img src="https://joss.theoj.org/papers/3e3e853bf4eb3ea9e754228343ad6c8b/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/3e3e853bf4eb3ea9e754228343ad6c8b/status.svg)](https://joss.theoj.org/papers/3e3e853bf4eb3ea9e754228343ad6c8b)

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

@awvwgk @dandavies99, 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 @lucydot 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 @awvwgk

✨ 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

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

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?
    Note: license is not included in source distribution on PyPI (License file is not included in sdist symmy596/SurfinPy#21)
  • Contribution and authorship: Has the submitting author (@jstse) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @dandavies99

✨ 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

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

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?
  • Contribution and authorship: Has the submitting author (@jstse) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
@whedon
Copy link
Author

whedon commented Dec 20, 2021

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @awvwgk it looks like you're currently assigned to review this paper 🎉.

⚠️ JOSS reduced service mode ⚠️

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.

⭐ 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:

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
Copy link
Author

whedon commented Dec 20, 2021

Wordcount for paper.md is 686

@whedon
Copy link
Author

whedon commented Dec 20, 2021

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.01210 is OK
- 10.1021/acs.jpcc.0c07437 is OK
- 10.1039/D0CP00021C is OK
- 10.1021/acs.jpcc.9b09046 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@whedon
Copy link
Author

whedon commented Dec 20, 2021

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.78 s (132.8 files/s, 411197.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          22            365           1079           1318
YAML                            11              6             12           1039
Jupyter Notebook                28              0         315114            804
reStructuredText                25            509            691            391
Markdown                         8            109              0            276
TeX                              6             31              0            247
DOS Batch                        1              8              1             27
CSS                              1              3              0             12
make                             1              4              6             10
Bourne Shell                     1              1              0              5
-------------------------------------------------------------------------------
SUM:                           104           1036         316903           4129
-------------------------------------------------------------------------------


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

Author                     Commits    Insertions      Deletions    % of changes
Adam Symington                  19          2431           1307            9.86
Benjamin Morgan                  6            45             20            0.17
jstse                           10           107             94            0.53
symmy596                        70         17803          16119           89.44

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
Adam Symington             2665          109.6         20.7                6.90
jstse                        97           90.7          3.7               10.31

@whedon
Copy link
Author

whedon commented Dec 20, 2021

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@lucydot
Copy link

lucydot commented Dec 20, 2021

Hi @jstse @awvwgk @dandavies99 just a heads up that I will be on annual leave for the next fortnight (until 3rd January). I will check in next week, but my response time will be slower than usual during this period.

@dandavies99 for some reason you weren't assigned as a reviewer on this thread but I will add your tick list manually now.

@lucydot
Copy link

lucydot commented Dec 20, 2021

/ooo December 20 until January 3

@lucydot
Copy link

lucydot commented Dec 20, 2021

@whedon add @dandavies99 as reviewer

@whedon
Copy link
Author

whedon commented Dec 20, 2021

OK, @dandavies99 is now a reviewer

@dandavies99
Copy link

Hi @jstse - The code, docs and examples are looking really good to me. I've just got a few points for improvement:

Installation

I think the installation instructions probably need another pass, largely because of complications caused by pymatgen versions. I opened a separate issue for this because it may need a bit of extra discussion.

Functionality

The functionality of the code is as advertised - there are a few minor points which came to light while going through the (excellent) tutorials - I've opened another separate issue.

Documentation

...is very clear! Might be worth providing an obvious link to the readthedocs hosted version in the README as it wasn't initially obvious to me that these were available - the badge is easy to miss.

Software paper

The paper looks good and ticks most boxes. Just a few suggestions:

  • My main concern is that there is a fair bit of repetition, so it could be made more concise and therefore accessible. For example, it is mentioned ~five times that this release expands functionality to bulk phases. Possibly the numbered list of new features is the best place to keep this, and the following paragraph could simply highlight the temperature range improvement? It probably doesn't need repeating again in the Statement of Need.

  • Any mention of the state of the field is missing. For example, you probably need to include how the new functionality for bulk phases is different to what is in the pymatgen.analysis.phase_diagram module, which has a PhaseDiagram class with lots of methods to do similar things to this. ASE also has some phase diagram capabilities.

  • Related to the above, probably a couple of citations needed beyond self-citations. These might be to existing software (pymatgen, ASE) but also a key reference or two to the main theory or theories that SurfinPy is using, if you think that's appropriate.

  • Some other minor suggestions from me but perhaps these are more editorial considerations for @lucydot:

    • Para 1: "can and have been used" -> "have been used"
    • "This method uses the vibrational modes for each structure to calculate the zero point energy and thus the vibrational entropy." - it's a bit unclear what "each structure" refers to.
    • "and applies a higher level of theory"- is a bit vague? Maybe you don't need this.

@lucydot
Copy link

lucydot commented Jan 1, 2022

Hi @dandavies99 thank-you for your quick and thoughtful review 👍

Hi @jstse - RE: the paper. I agree with the suggestions @dandavies99 makes above, in addition:

  • "in order to improve performance": this is a little vague. Is this a speed-up compared to the previous software release(s)?
  • "Similarly, to the surface module,": the wording here doesn't sound right: perhaps re-phrased to "Similar to the the surface module". Although, re-iterating Dan's point, you may want to do a larger re-structuring of this section to avoid repetition.

State of the field, and how your package compares to other packages with an overlap of functionality (pymatgen/ASE) is important. I'll include the relevant quote from the author guidelines:

Your software should be a significant contribution to the available open source software that either enables some new research challenges to be addressed or makes addressing research challenges significantly better (e.g., faster, easier, simpler).

If you have any questions or if I can clarify anything, please let me know.

@whedon
Copy link
Author

whedon commented Jan 3, 2022

👋 @awvwgk, please update us on how your review is going (this is an automated reminder).

@awvwgk
Copy link

awvwgk commented Jan 3, 2022

Installation

Documentation

Generally, in a really good shape

Software paper

  • state of the field should be discussed in more details
  • is Vasp the only available backend for this project? What about other (free) electronic structure programs?

@lucydot
Copy link

lucydot commented Jan 10, 2022

Hello @jstse, pinging you just in case you have missed the feedback from @awvwgk above ☝️.

It looks like you have a few items to be working on, please also let us know how your progress is going here in the thread.

@jstse
Copy link

jstse commented Jan 19, 2022

Thank you for reviewing the paper so quickly, I am currently working with my collaborators on implementing the suggestions provided. Unfortunately I am out of the country until Monday and plan to implement most corrections next week when I get back

@lucydot
Copy link

lucydot commented Feb 4, 2022

Hello @jstse do you have any updates for us? It would good to have a feel for your progress on the points raised by the reviewers.

@jstse
Copy link

jstse commented Feb 7, 2022

We are almost complete with the majority of the changes. I have issued a pull request this morning to the code base and double checking the changes before merging. I will post an detailed update and regernerate the paper when the merge has been complete.

@jstse
Copy link

jstse commented Feb 8, 2022

Hi, The pull request has now been merged and will give a detailed response below:

Installation

  • Removed specific versions of requirements causing issues in installation. After checking became apparent that we do not need specific versions of these codes. (Waiting on confirmation that this has solved installation issues before closing issue)
  • As suggested also cleaned up installation instructions
  • Removed Coverage and Coveralls from install requirements
  • Moved Jupyter to extra requirements and updated insatllation instructions in readme to include insatlling version with Jupyter.
  • PyPI mismatch tags, currently waiting for access to the PyPI repository.
  • PyPI License not included, currently waiting for access to the PyPI repository.

Functionality

  • Discrepancies in the tutorials should now be fixed, issues were due to pictures in tutorials not being updated after changing plotting values. (Waiting for confirmation before closing issue)

Documentation

  • As suggested, we have now added a link to the compiled documentation in the Readme.
  • Accepted pull request correcting typos in documentation

Software paper

Dan

My main concern is that there is a fair bit of repetition, so it could be made more concise and therefore accessible. For example, it is mentioned ~five times that this release expands functionality to bulk phases. Possibly the numbered list of new features is the best place to keep this, and the following paragraph could simply highlight the temperature range improvement? It probably doesn't need repeating again in the Statement of Need.

  • Removed list and went into details in the paragraphs after the list of major contributions. This should remove all the repetition. We also have removed the repetition in the statement of need.

Any mention of the state of the field is missing. For example, you probably need to include how the new functionality for bulk phases is different to what is in the pymatgen.analysis.phase_diagram which has a Phase Diagram class with lots of methods to do similar things to this. ASE also has some phase diagram capabilities.

  • We have now included a sentence acknowledging the other software, and we have highlighted the main advantage of our code. This is added to the statement of need.

Related to the above, probably a couple of citations needed beyond self-citations. These might be to existing software (pymatgen, ASE) but also a key reference or two to the main theory or theories that SurfinPy is using, if you think that's appropriate.

  • We have accounted the references for the software.

Para 1: "can and have been used" -> "have been used"

  • Correction accepted.

This method uses the vibrational modes for each structure to calculate the zero point energy and thus the vibrational entropy." - it's a bit unclear what "each structure" refers to.

  • We have now been consistent with terminology and used "solid bulk phases".

"and applies a higher level of theory"- is a bit vague? Maybe you don't need this.

  • This has now been removed.

Lucy

"in order to improve performance": this is a little vague. Is this a speed-up compared to the previous software release(s)?

  • We have specified that the improved performance is both in terms of capability and speed up.

"Similarly, to the surface module,": the wording here doesn't sound right: perhaps re-phrased to "Similar to the the surface module". Although, re-iterating Dan's point, you may want to do a larger re-structuring of this section to avoid repetition.

  • correction accepted.

State of the field, and how your package compares to other packages with an overlap of functionality (pymatgen/ASE) is important. I'll include the relevant quote from the author guidelines:
Your software should be a significant contribution to the available open source software that either enables some new research challenges to be addressed or makes addressing research challenges significantly better (e.g., faster, easier, simpler).

  • We have now addressed this point in the statement of need.

AWVWGK

state of the field should be discussed in more details

  • We have added a comment in the statement of need as also suggested by the editor and other reviewer.

is Vasp the only available backend for this project? What about other (free) electronic structure programs?

  • No. The code only needs a set of self-contained energies. They can be derived using an ab initio code or a code that implements classical techniques based on force fields. We have indeed added this to the main paper.

@lucydot
Copy link

lucydot commented Feb 10, 2022

Thanks for the update summary @jstse.

@dandavies99 @awvwgk - over to you now, please let us know if the latest changes have answered the points raised during your review ✅

@editorialbot
Copy link
Collaborator

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

@lucydot
Copy link

lucydot commented Mar 2, 2022

@editorialbot commands

@editorialbot
Copy link
Collaborator

Hello @lucydot, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

# Remind an author or reviewer to return to a review after a 
# certain period of time (supported units days and weeks)
@editorialbot remind @reviewer in 2 weeks

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for version
@editorialbot set v1.0.0 as version

# Set a value for archive
@editorialbot set 10.21105/zenodo.12345 as archive

# Set a value for branch
@editorialbot set joss-paper as branch

# Generates the pdf paper
@editorialbot generate pdf

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Flag submission with questionable scope
@editorialbot query scope

# Get a link to the complete list of reviewers
@editorialbot list reviewers

# Open the review issue
@editorialbot start review

@lucydot
Copy link

lucydot commented Mar 2, 2022

@editorialbot set 10.5281/zenodo.6250513 as archive

@editorialbot
Copy link
Collaborator

Done! archive is now 10.5281/zenodo.6250513

@lucydot
Copy link

lucydot commented Mar 2, 2022

@editorialbot set v2.0.2 as version

@editorialbot
Copy link
Collaborator

Done! version is now v2.0.2

@lucydot
Copy link

lucydot commented Mar 2, 2022

@editorialbot recommend-accept

@editorialbot
Copy link
Collaborator

Attempting dry run of processing paper acceptance...

@editorialbot editorialbot added the recommend-accept Papers recommended for acceptance in JOSS. label Mar 2, 2022
@editorialbot
Copy link
Collaborator

⚠️ Error preparing acceptance.

@lucydot
Copy link

lucydot commented Mar 2, 2022

@editorialbot generate pdf

@symmy596
Copy link

symmy596 commented Mar 2, 2022

The error is due to me not having an affiliation. The work was completed while I was at the University of Bath so I have altered the paper to include that as my affiliation.

@editorialbot
Copy link
Collaborator

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@lucydot
Copy link

lucydot commented Mar 2, 2022

@editorialbot recommend-accept

@editorialbot
Copy link
Collaborator

Attempting dry run of processing paper acceptance...

@editorialbot
Copy link
Collaborator

👋 @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof 👉 openjournals/joss-papers#3008

If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3008, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

@kyleniemeyer
Copy link

Hi @symmy596, I just submitted a minor PR that fixes some formatting things with the paper: symmy596/SurfinPy#29

Could you merge that? Then, I can accept the paper.

@symmy596
Copy link

symmy596 commented Mar 3, 2022

PR merged
Thanks for the easy fix!

@kyleniemeyer
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@kyleniemeyer
Copy link

@editorialbot accept

@editorialbot
Copy link
Collaborator

Doing it live! Attempting automated processing of paper acceptance...

@editorialbot editorialbot added accepted published Papers published in JOSS labels Mar 3, 2022
@editorialbot
Copy link
Collaborator

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@editorialbot
Copy link
Collaborator

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.04014 joss-papers#3014
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.04014
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? Notify your editorial technical team...

@kyleniemeyer
Copy link

Congratulations @jstse on your article's publication in JOSS!

Many thanks to @awvwgk and @dandavies99 for reviewing this, and @lucydot for editing.

@editorialbot
Copy link
Collaborator

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

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

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.04014/status.svg
   :target: https://doi.org/10.21105/joss.04014

This is how it will look in your documentation:

DOI

We need your help!

The 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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review TeX
Projects
None yet
Development

No branches or pull requests

8 participants