whedon commented 3 years ago

Submitting author: @brlec (Katarina Brlec) Repository: https://github.com/SMTG-UCL/surfaxe Version: v1.2 Editor: @danielskatz Reviewer: @pzarabadip, @eihernan Archive: 10.5281/zenodo.4776449

: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 badge code:

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

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

@pzarabadip & @eihernan, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
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 @danielskatz 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 @pzarabadip

Conflict of interest

[x] 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

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@brlec) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] 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?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] 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 @eihernan

Conflict of interest

[x] 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

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@brlec) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] 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?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] 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 commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @pzarabadip, @eihernan 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:

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

watching

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 3 years ago

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

OK DOIs

- 10.1002/cpe.3505 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.21105/joss.01944 is OK
- 10.1016/j.commatsci.2020.110086 is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=37.05 s (1.9 files/s, 32345.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
XML                              8              0              0        1191898
Python                          27            629            936           2272
JSON                             3              0              0            360
reStructuredText                10             74            106            147
Markdown                         2             72              0            145
YAML                            13              4              0            117
Jupyter Notebook                 4              0           1436            102
TeX                              1              3              0             78
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            70            794           2486        1195154
-------------------------------------------------------------------------------

Statistical information for the repository '18472d3ed532dce818f85b45' was
gathered on 2021/04/14.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Dan Davies                       3            93              2            0.91
Daniel Davies                   29           200             96            2.83
Sean Kavanagh                    1             1              1            0.02
brlec                          104          6685           3376           96.24

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
Dan Davies                  214          230.1          4.1               16.36
brlec                      3623           54.2          2.9                7.09

whedon commented 3 years ago

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

danielskatz commented 3 years ago

👋 @pzarabadip and @eihernan - Thanks for agreeing to review this submission. This is the review thread for the paper. All of our communications will happen here from now on.

Both reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

Please read the first couple of comments in this issue carefully, so that you can accept the invitation from JOSS and be able to check items, and so that you don't get overwhelmed with notifications from other activities in JOSS.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#3171 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if either of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@danielskatz) if you have any questions/concerns.

whedon commented 3 years ago

:wave: @pzarabadip, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @eihernan, please update us on how your review is going (this is an automated reminder).

danielskatz commented 3 years ago

👋 @pzarabadip & @eihernan - How are your reviews going?

ezpzbz commented 3 years ago

Hi @danielskatz. Sorry for late response. I just finished my review on the code.

I would like to thank authors for developing surfaxe package which is nice and useful contribution for the community. I carefully went through code, tutorials, tests, documentation, and paper and found from my point view that the code/paper is (almost) ready to be accepted for publication in JOSS. I only have few suggestions that could be nice to be implemented before publication.

Applying formatting/linting tools on the code for which I opened an issue and explained it in more detail.
Another suggestion would be moving the CI to GitHub Actions to improve the sustainability in future.
From my personal experience: it would be beneficial for the performance to use OUTCAR parser of parsevasp package instead of the one provided by pymatgen.

Best regards, Pezhman

danielskatz commented 3 years ago

Thanks @pzarabadip - Do I understand correctly that these are suggestions, not changes that are required for you?

Or is the last one a requirement, tied to the fact that you didn't check the performance box in your review?

danielskatz commented 3 years ago

👋 @eihernan - Can you update us on the status of your review?

ezpzbz commented 3 years ago

Hi @danielskatz - That's correct, those are suggestions. I missed to check the performance box. The last one can improve the performance for large files but nothing against current performance of the code.

eihernan commented 3 years ago

Hi @danielskatz, I've been a bit behind on this. I'll update with progress shortly.

danielskatz commented 3 years ago

ok - it's great to hear from you that you're still workin on this, and we'll look forward to a further update

brlec commented 3 years ago

Hi @pzarabadip,

Thank you for the thorough review!

To answer the suggestions:

Applying formatting/linting tools: We have removed the unused imports in the latest commit on the develop branch and they will be merged with master once JOSS review is complete. We have added linting tools that should catch unused imports going forward.
Moving the CI to GitHub Actions: We have implemented this in the latest commit on the master branch, the tests are passing.
Using parsevasp: OUTCARs are parsed for two different purposes in surfaxe - getting timing information from convergence calculations or getting the core level energies. We have tested parsevasp vs pymatgen and found that parsevasp offers a 10x speedup for reading the timing information.
```
from parsevasp import outcar
from pymatgen.io.vasp.outputs import Outcar
import time
```

pymatgen

start = time.time() otc = Outcar('surfaxe/example_data/convergence/Y2Ti2S2O5/001/30_50_15/OUTCAR') times = otc.run_stats print(time.time() - start)

0.21026110649108887

parsevasp

start = time.time() otc = outcar.Outcar(file_path='surfaxe/example_data/convergence/Y2Ti2S2O5/001/30_50_15/OUTCAR') times = otc.get_run_stats() print(time.time() - start)

0.029942989349365234


However, the additional wait time for the user arising from `pymatgen` parser would be minimal as parsing of vasprun.xml takes far longer. `parsevasp` OUTCAR parser also cannot read in the core level energies (yet), so we are going to stick with `pymatgen` as the package for reading in OUTCARs. We will definitely look into using `parsevasp` in the future if parsing of core level energies is added.

Thanks again for the really helpful review! Katarina

eihernan commented 3 years ago

Hi @danielskatz, here is an update of my review status.

I have gone through the docs, tutorials, and paper and find them to be clear in their purpose, well-written and easy to follow. The inclusion of a binder environment is appreciated and the notebooks are well documented to explain the limitations when testing on that platform. The tutorials illustrate the functionality of the package well.

I've been able to install and use the package on my personal computer following the installation instructions, but ran into an issue with the CLI not working when installing in a virtual environment. I want to make sure this was (my) user error before I check off the installation box.

Thank you to the authors for their work, I think this package goes a long way to making surface calculation workflows more robust and reproducible. A suggestion for now:

The package does rely on pymatgen to generate the VASP input (which is fine), however, it's not obvious from the documentation if the package is fully functional without setting up pymatgen to run with VASP or that you can customize its behavior. In particular, my group would not be able to use the make_input_files feature out-of-the-box because we don't keep a central repository of POTCAR files for use with pymatgen. I anticipate users who create their files locally and then run their jobs remotely would encounter this problem as well.
Along the note above, while the YAML documentation is clear, the generation of the configuration dictionaries would do well to be documented, or at least point to the relevant pymatgen documentation.

Best, Erick

danielskatz commented 3 years ago

Thanks @eihernan - re the installation problem, if it does continue, you might open an issue in https://github.com/SMTG-UCL/surfaxe and mention https://github.com/openjournals/joss-reviews/issues/3171 there so we can track it.

And I'll look forward to a response from the authors to your other suggestions.

brlec commented 3 years ago

Hi @eihernan,

Thank you for your review and kind words.

Thank you for raising the issue with CLI - we believe this was related to how yaml was imported in the CLI scripts. We have replaced yaml with ruamel.yaml which should work now even in blank virtual environments as ruamel.yaml is a dependency of pymatgen.

To respond to suggestions:

POTCAR set up for pymatgen: Pymatgen is fairly rigid with respect to how it can read input files, unfortunately this means the POTCARs need to be additionally set up to work with pymatgen. In response to the confusion this may cause we have added the following line to README, with a link to the pymatgen documentation that outlines how to complete the set up.

"For the code to generate VASP input files along with the surface slabs, POTCARs need to be set up with pymatgen"

We also added a similar line to the Installation section of surfaxe docs.

Additionally, we modified the code to check that PMG_VASP_PSP_DIR key exists in pymatgen.core.SETTINGS before structures are written to file. If POTCARs are not set up, only the structures are written to file and the following warning is raised: POTCAR directory not set up in pymatgen, only POSCARs were generated. We are open to suggestions on how this could be further improved.

Configuration dictionaries: We agree that for someone not familiar with pymatgen ecosystem this may be somewhat confusing. We added a section in the Tutorials section of documentation on the developbranch outlining how this should be used.

All changes were made on the develop branch if you would like to try re-installing the package in a virtual environment to see if the CLI problem is fixed now?

Thank you again, Katarina

eihernan commented 3 years ago

Hi @brlec, I think the additions to the documentation are clear on what a user should do to fully generate VASP inputs. Since the functionality this package provides (creating appropriate file-structures and slabs) is maintained, and this is generally code-agnostic, I think this is a fine solution.

I installed the develop branch, and the CLI is working as expected in a virtual environment.

@danielskatz with the changes above and installation working as expected, I believe this meets the standards for inclusion in JOSS.

Thank you again to the developers.

danielskatz commented 3 years ago

Thanks @eihernan!

danielskatz commented 3 years ago

@brlec - at this point (or once you are ready if there are any further changes you want to make, such as in response to reviewer suggestions), can you:

[ ] Make a tagged release of your software, and list the version tag of the archived version here.
[ ] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
[ ] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
[ ] Please list the DOI of the archived version here.

I can then move forward with accepting the submission, which will include a proof read.

brlec commented 3 years ago

Hi @danielskatz - the JOSS release is tagged v1.2 and was archived on Zenodo with the correct metadata. The DOI is 10.5281/zenodo.4776449

danielskatz commented 3 years ago

@whedon set v1.2 as version

whedon commented 3 years ago

OK. v1.2 is the version.

danielskatz commented 3 years ago

@whedon set 10.5281/zenodo.4776449 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4776449 is the archive.

danielskatz commented 3 years ago

@whedon accept

whedon commented 3 years ago

Attempting dry run of processing paper acceptance...

whedon commented 3 years ago

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

OK DOIs

- 10.1002/cpe.3505 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.21105/joss.01944 is OK
- 10.1016/j.commatsci.2020.110086 is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 3 years ago

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

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2328

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2328, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true

danielskatz commented 3 years ago

@brlec - I'm suggesting some small changes to the paper in https://github.com/SMTG-UCL/surfaxe/pull/11

Please either merge this or let me know what you disagree with.

brlec commented 3 years ago

@danielskatz we've merged all the suggestions

danielskatz commented 3 years ago

@whedon accept

whedon commented 3 years ago

Attempting dry run of processing paper acceptance...

whedon commented 3 years ago

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

OK DOIs

- 10.1002/cpe.3505 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.21105/joss.01944 is OK
- 10.1016/j.commatsci.2020.110086 is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 3 years ago

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

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2329

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2329, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true

danielskatz commented 3 years ago

@whedon accept deposit=true

whedon commented 3 years ago

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

whedon commented 3 years ago

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

whedon commented 3 years ago

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

Here's what you must now do:

Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/2330
Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.03171
If everything looks good, then close this review issue.
Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? Notify your editorial technical team...

danielskatz commented 3 years ago

Congratulations to @brlec (Katarina Brlec) and co-authors!!

And thanks to @pzarabadip and @eihernan for reviewing.

Everyone, this was a really smooth and easy process - thanks again!

whedon commented 3 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

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

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

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

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: https://joss.theoj.org/reviewer-signup.html
Making a small donation to support our running costs here: https://numfocus.org/donate-to-joss

openjournals / joss-reviews

[REVIEW]: Surfaxe: Systematic surface calculations #3171

Status

Reviewer instructions & questions

Review checklist for @pzarabadip

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @eihernan

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

pymatgen

parsevasp