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]: ShakeNBreak: Navigating the defect configurational landscape #4817

Closed
editorialbot opened this issue Oct 5, 2022 · 55 comments
Closed
Assignees
Labels
accepted published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review Shell TeX Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Oct 5, 2022

Submitting author: @ireaml (Irea Mosquera-Lois)
Repository: https://github.com/SMTG-UCL/ShakeNBreak
Branch with paper.md (empty if default branch):
Version: 22.11.6
Editor: @rkurchin
Reviewers: @obaica, @mkhorton
Archive: 10.5281/zenodo.7377173

Status

status

Status badge code:

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

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

@obaica & @mkhorton, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review.
First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @rkurchin 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

Checklists

📝 Checklist for @mkhorton

📝 Checklist for @obaica

@editorialbot editorialbot added Python review Shell TeX Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials labels Oct 5, 2022
@editorialbot
Copy link
Collaborator Author

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

OK DOIs

- 10.1038/s41524-021-00537-1 is OK
- 10.1088/0953-8984/23/5/053201 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.1103/PhysRevB.54.11169 is OK
- 10.1088/0953-8984/21/39/395502 is OK
- 10.1524/zkri.220.5.567.65075 is OK
- 10.1016/j.cpc.2009.06.022 is OK
- 10.1063/5.0007045 is OK
- 10.1021/acsenergylett.1c00380 is OK
- 10.1039/d2fd00043a is OK
- 10.48550/ARXIV.2207.09862 is OK
- 10.1016/j.cpc.2018.01.004 is OK
- 10.1016/j.commatsci.2016.12.040 is OK
- 10.1088/1674-4926/43/4/042101 is OK
- 10.1016/j.cpc.2021.107946 is OK
- 10.1016/j.matt.2021.06.003 is OK
- 10.1016/j.commatsci.2020.110086 is OK
- 10.1002/cpe.3505 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@editorialbot
Copy link
Collaborator Author

Software report:

github.com/AlDanial/cloc v 1.88  T=0.32 s (254.2 files/s, 105429.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          20           1250           3123          13542
SVG                              2              0             34           2809
YAML                            30             47             55            582
reStructuredText                18            262            296            428
Jupyter Notebook                 2              0          10313            269
TeX                              1              1              0            226
Markdown                         2             34              0            161
Bourne Shell                     2              8              8             85
DOS Batch                        1              8              1             26
make                             1              4              7              9
TOML                             1              0              0              6
JSON                             1              0              0              1
-------------------------------------------------------------------------------
SUM:                            81           1614          13837          18144
-------------------------------------------------------------------------------


gitinspector failed to run statistical information for the repository

@editorialbot
Copy link
Collaborator Author

Wordcount for paper.md is 800

@editorialbot
Copy link
Collaborator Author

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

@rkurchin
Copy link

👋 just checking in with reviewers @obaica, @mkhorton – let me know if you have any questions about how to get your review started!

@rkurchin
Copy link

Pinging @obaica and @mkhorton on this again!

@mkhorton
Copy link

mkhorton commented Oct 20, 2022

Review checklist for @mkhorton

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 https://github.com/SMTG-UCL/ShakeNBreak?
  • 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 (@ireaml) 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
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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, who the target audience is, and its relation to other work?
  • 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?

@rkurchin
Copy link

Thanks for getting started, @mkhorton, and a reminder to @obaica to do the same! Also, you should feel free to open issues in the software repo if you come across anything while reviewing – try to remember to link to this issue for easy tracking.

@rkurchin
Copy link

rkurchin commented Nov 7, 2022

Checking in again with reviewers, @mkhorton and @obaica!

@mkhorton
Copy link

Hi @rkurchin, I've finished my review. This is an excellent, high-quality code, well-motivated and well-documented--I think it's an exemplar for the community, and I would be happy to recommend publication in JOSS.

In terms of the reproducibility checkbox for the data presented in the paper, for full reproducibility, I would have to re-run the DFT calculations myself, but I don't think this is really required: I'm satisfied the code itself does what it claims, and testing the DFT relaxation seems unnecessary.

For the authors @ireaml et al., I would make these some comments:

  • Since ShakeNBreak uses the pymatgen Defect class, I would make a connection with @jmmshn who is working on pymatgen-analysis-defects. This seems like a complementary effort and perhaps there'd be some mutual benefit from talking.
  • As a user of a code, I find integrated bash scripts (e.g., via snb.run) to be cumbersome, since they are doing a similar job to a workflow management system but in a custom way that, in previous experience with other codes that adopt a similar system, usually ends up being quite brittle. I would love to see integration with e.g. atomate2 or AiiDA to make integrating ShakeNBake into existing workflows easier, and make scaling it easier too. I note that atomate2 has a run_locally option that could be used too, since I recognise there is benefit to having an option to run locally, or in a debugging context, etc.
  • I didn't create issues for these, but only two hiccups, (1) importlib_metadata isn't in the setup.py, so required an additional install to run the notebook (I'm not sure if this is strictly required either, since this is in the stdlib as of Python 3.8), and (2) I was not expecting installing this package to also install a font! I think this is harmless, but perhaps worth mentioning in the README, especially since the Montserrat font is distributed under the SIL license, which I believe requires attribution. If we need a longer discussion on either of these points, let's create an issue to discuss.

Overall, really really excellent! The GIF in the docs is especially a nice touch :)

Thanks for your patience while I prepared this review.

@jmmshn
Copy link

jmmshn commented Nov 16, 2022

Thanks, @mkhorton, we have been talking already and I'm super interested in incorporating this into the defects workflow in atomate2.

@rkurchin
Copy link

Thanks so much @mkhorton for your review and thoughtful comments!

Pinging @obaica again for review progress...

@obaica
Copy link

obaica commented Nov 25, 2022

Review checklist for @obaica

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 https://github.com/SMTG-UCL/ShakeNBreak?
  • 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 (@ireaml) 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
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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, who the target audience is, and its relation to other work?
  • 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?

@obaica
Copy link

obaica commented Nov 25, 2022

Hi @rkurchin, I've finished my review. This is an excellent and easy-to-use (e.g., includes a user-friendly command line interface) code that provides new insights for the defect research community. With this tool, I believe it will enable researchers to gain a deeper and more accurate understanding of defect-related properties, and I would highly recommend it for publication in JOSS.

For the authors @ireaml et al., some comments are listed below.

  • The authors should provide some specific input files (e.g., CdTe_Bulk_Supercell_POSCAR, CdTe_V_Cd_POSCAR) that are mentioned in the [tutorials](https://github.com/SMTG-UCL/ShakeNBreak/tree/main/tutorials) .
  • Interstitial defects are relatively complex compared to vacancy and substitution defects. I suggest that the authors provide a specific example of how to first generate interstitial defects using pymatgen.analysis.defects or doped/PyCDT, etc., and then further process them with ShakeNBreak.
  • In the Section Generate defects with pymatgen.analysis.defects of the [Tutorials](https://shakenbreak.readthedocs.io/en/latest/ShakeNBreak_Example_Workflow.html) , the comment sentence "# Chech defect fractional coordinates" should be "# Check defect fractional coordinates".

Finally, I hope that more researchers in the defect community will use this tool for their research, or even combine ShakeNBreak and [nonrad](https://github.com/mturiansky/nonrad) for a more in-depth study of the nature of defects. I will also promote this tool to the researchers around me.

Thank you very much for your patience due to my delay in this review process.

@ireaml
Copy link

ireaml commented Nov 25, 2022

Hi @mkhorton, thanks for the helpful review!

Regarding the comments:

  1. We think that refactoring snb.run using a workflow manager might not be worthy in our case. Given the initial learning curve/database setup required for aiida/atomate, this might become a barrier for some users. However, we agree that for high-throughput applications, integration with a workflow manager will be helpful, so we'll add a notebook showing how this can be done.
  2. Good points!
    • We have now added importlib_metadata as a requirement (commit 71f5bc57).

    • And a comment after the installation instructions that the Montserrat font is installed and used by default for plotting (commit e466a83). The FAQ page of the SIL font license states:

      1. Can the fonts be included with Free/Libre and Open Source Software collections such as GNU/Linux and BSD distributions and repositories?
        Yes! Fonts licensed under the OFL can be freely included alongside other software under FLOSS (Free/Libre and Open Source Software) licenses. (...)
      2. Is any kind of acknowledgement required?
        No. (...)

      So I think we should be fine, but good catch!

Thanks again! :)

@ireaml
Copy link

ireaml commented Nov 25, 2022

Hi @obaica , thanks for the detailed review - good catch on the typo!

  1. Input files are provided in the tests/data/vasp/CdTe directory
  2. We have now added a second example notebook, showing how to generate interstitial defects using Doped and apply ShakeNBreak to them. (commit 536307a6)
  3. Now corrected with commit 6bbcebd0

Thanks again! :)

@rkurchin
Copy link

Great, looks like we're almost ready to rock here! Thanks everyone! @ireaml, I'll do an editorial pass over the manuscript and send any comments shortly. In the meantime, the next steps for you are:

  1. Merge any and all changes from this review into your main branch and issue a new version tag. (If you want to merge in the paper, you may, but it is not required that the actual manuscript live into the repo in perpetuity since JOSS will host it and you can simply add a badge link or whatever you like. But the actual changes to software and docs do need to be merged!)
  2. Create a DOI for the contents of the repo at the same commit corresponding to that version tag, e.g. using figshare or Zenodo. Please make sure that the metadata (version number, title, author list, etc.) match those of your manuscript.
  3. Post a comment here with the version number and DOI.

@rkurchin
Copy link

@editorialbot check references

@editorialbot
Copy link
Collaborator Author

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

OK DOIs

- 10.1038/s41524-021-00537-1 is OK
- 10.1088/0953-8984/23/5/053201 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1088/1361-648X/aa680e is OK
- 10.1103/PhysRevB.54.11169 is OK
- 10.1088/0953-8984/21/39/395502 is OK
- 10.1524/zkri.220.5.567.65075 is OK
- 10.1016/j.cpc.2009.06.022 is OK
- 10.1063/5.0007045 is OK
- 10.1021/acsenergylett.1c00380 is OK
- 10.1039/d2fd00043a is OK
- 10.48550/ARXIV.2207.09862 is OK
- 10.1016/j.cpc.2018.01.004 is OK
- 10.1016/j.commatsci.2016.12.040 is OK
- 10.1088/1674-4926/43/4/042101 is OK
- 10.1016/j.cpc.2021.107946 is OK
- 10.1016/j.matt.2021.06.003 is OK
- 10.1016/j.commatsci.2020.110086 is OK
- 10.1002/cpe.3505 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@rkurchin
Copy link

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

@rkurchin
Copy link

Some minor editorial suggestions:

  • line 11: add comma after "site"
  • line 12 -> 10: move "however" to the beginning of the sentence – i.e. "However, the standard modeling approach..."
  • 16, 20, 27, 48: hyphenate "low-energy"
  • 26, 49: hyphenate "energy-lowering"
  • 27: add comma after "efficient"
  • 29: "automatise" -> "automate"
  • 39: unhyphenate "readily applicable"
  • 51: add hyphen after "site" i.e. "analyse site- and orbital-decomposed..."

Otherwise, looks good!

@editorialbot
Copy link
Collaborator Author

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

Check final proof 👉📄 Download article

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

@editorialbot editorialbot added the recommend-accept Papers recommended for acceptance in JOSS. label Nov 29, 2022
@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Nov 30, 2022

@ireaml I am the AEiC for this track, and here to help process this work for acceptance in JOSS. I checked your repository, the paper, and the archive and all seems in order. I have only some minor points on the paper which I hope you can address:

  • Please spell out UK as United Kingdom for all affiliations
  • You may remove street names and postal codes for all affiliations, simply having Institute/department, University, City, Country is sufficient.

@ireaml
Copy link

ireaml commented Dec 1, 2022

@editorialbot generate pdf

@editorialbot
Copy link
Collaborator Author

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

@ireaml
Copy link

ireaml commented Dec 1, 2022

Hi @Kevin-Mattheus-Moerman , thanks for the suggestions! They have been addressed with 49097747.

@Kevin-Mattheus-Moerman
Copy link
Member

@ireaml thanks, all system go so.

@Kevin-Mattheus-Moerman
Copy link
Member

@editorialbot accept

@editorialbot
Copy link
Collaborator Author

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

@editorialbot
Copy link
Collaborator Author

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

@editorialbot
Copy link
Collaborator Author

🐘🐘🐘 👉 Toot for this paper 👈 🐘🐘🐘

@editorialbot
Copy link
Collaborator Author

🚨🚨🚨 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.04817 joss-papers#3769
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.04817
  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...

@editorialbot editorialbot added accepted published Papers published in JOSS labels Dec 1, 2022
@ireaml
Copy link

ireaml commented Dec 1, 2022

Hi @rkurchin , thanks for editing!
Just noticed that one of the references was showing the author initial in the text, so fixed this with 3f22b77e. Is it still possible to update the final PDF?
Sorry about this!

@Kevin-Mattheus-Moerman
Copy link
Member

@openjournals/dev can you help address this? ☝️

@xuanxu
Copy link
Member

xuanxu commented Dec 1, 2022

@Kevin-Mattheus-Moerman You can regenerate the final pdf reaccepting the paper via @editorialbot reaccept

@Kevin-Mattheus-Moerman
Copy link
Member

@editorialbot reaccept

@editorialbot
Copy link
Collaborator Author

Rebuilding paper!

@editorialbot
Copy link
Collaborator Author

🌈 Paper updated!

New PDF and metadata files 👉 openjournals/joss-papers#3771

@Kevin-Mattheus-Moerman
Copy link
Member

@ireaml congratulations on this publication!

Thanks for editing @rkurchin!

And special thanks to the reviewers: @obaica and @mkhorton

@editorialbot
Copy link
Collaborator Author

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

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

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

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:

@mkhorton
Copy link

mkhorton commented Dec 1, 2022

Congratulations @ireaml et al.! :) Thanks @rkurchin for guiding this review

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 Shell TeX Track: 2 (BCM) Biomedical Engineering, Biosciences, Chemistry, and Materials
Projects
None yet
Development

No branches or pull requests

8 participants