joss-reviews
joss-reviews copied to clipboard
[REVIEW]: ShakeNBreak: Navigating the defect configurational landscape
Submitting author: @ireaml (Irea Mosquera-Lois) Repository: https://github.com/SMTG-UCL/ShakeNBreak Branch with paper.md (empty if default branch): Version: v1.0.0 Editor: @rkurchin Reviewers: @obaica, @mkhorton Archive: Pending
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: [](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
@obaica, please create your checklist typing: @editorialbot generate my checklist
@mkhorton, please create your checklist typing: @editorialbot generate my checklist
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
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
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
Wordcount for paper.md
is 800
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
👋 just checking in with reviewers @obaica, @mkhorton – let me know if you have any questions about how to get your review started!
Pinging @obaica and @mkhorton on this again!
Review checklist for @mkhorton
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 https://github.com/SMTG-UCL/ShakeNBreak?
- [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 (@ireaml) 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
- [x] 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.
- [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
- [x] 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
- [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, who the target audience is, and its relation to other work?
- [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?
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.
Checking in again with reviewers, @mkhorton and @obaica!
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 onpymatgen-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 arun_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 thesetup.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.
Thanks, @mkhorton, we have been talking already and I'm super interested in incorporating this into the defects workflow in atomate2.
Thanks so much @mkhorton for your review and thoughtful comments!
Pinging @obaica again for review progress...
Review checklist for @obaica
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 https://github.com/SMTG-UCL/ShakeNBreak?
- [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 (@ireaml) 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
- [x] 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.
- [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
- [x] 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
- [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, who the target audience is, and its relation to other work?
- [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?
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
ordoped/PyCDT
, etc., and then further process them withShakeNBreak
. - 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.
Hi @mkhorton, thanks for the helpful review!
Regarding the comments:
- 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 foraiida
/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. - 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:
- 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. (...)
- Is any kind of acknowledgement required? No. (...)
So I think we should be fine, but good catch!
-
Thanks again! :)
Hi @obaica , thanks for the detailed review - good catch on the typo!
- Input files are provided in the
tests/data/vasp/CdTe
directory - We have now added a second example notebook, showing how to generate interstitial defects using
Doped
and applyShakeNBreak
to them. (commit 536307a6) - Now corrected with commit 6bbcebd0
Thanks again! :)
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:
- 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!)
- 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.
- Post a comment here with the version number and DOI.
@editorialbot check references
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 generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
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 generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi @rkurchin , thank you for the detailed read and the comments! They have now been implemented.
The version number is v22.11.6
and the DOI is 10.5281/zenodo.7377173
().
@editorialbot set 22.11.6 as version
Done! version is now 22.11.6
@editorialbot set 10.5281/zenodo.7377173 as archive
Done! Archive is now 10.5281/zenodo.7377173