search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
707 stars 37 forks source link

[REVIEW]: LobsterPy: A package to automatically analyze LOBSTER runs #6286

Closed editorialbot closed 6 months ago

editorialbot commented 7 months ago

Submitting author: !--author-handle-->@JaGeo<!--end-author-handle-- (Janine George) Repository: https://github.com/JaGeo/LobsterPy Branch with paper.md (empty if default branch): Version: v0.3.8 Editor: !--editor-->@RMeli<!--end-editor-- Reviewers: @berquist, @srmnitc Archive: 10.5281/zenodo.10713348

Status

status

Status badge code:

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

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

@berquist & @srmnitc, 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 @RMeli 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 @berquist

📝 Checklist for @srmnitc

editorialbot commented 7 months ago

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 commented 7 months ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=6.64 s (12.1 files/s, 11022.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JSON                            12              0              0          55254
Python                          25           1716           1395          11457
Markdown                        12            120              0            438
YAML                             6             36             46            254
reStructuredText                18            172            225            175
TOML                             1             20              1            159
TeX                              1             15              0            157
Jupyter Notebook                 2              0           1231            141
CSS                              1             21             44             63
HTML                             2              0              0             28
-------------------------------------------------------------------------------
SUM:                            80           2100           2942          68126
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 7 months ago

Wordcount for paper.md is 720

editorialbot commented 7 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1021/j100135a014 is OK
- 10.1515/9783111167213 is OK
- 10.1021/acsomega.3c00395 is OK
- 10.1038/s41597-023-02477-5 is OK
- 10.1021/jacs.2c11908 is OK
- 10.1021/acs.chemmater.1c03349 is OK
- 10.1002/anie.198708461 is OK
- 10.1016/j.apsusc.2022.156064 is OK
- 10.1021/ja00349a027 is OK
- 10.1021/acs.jpcc.1c00718 is OK
- 10.1002/cplu.202200123 is OK
- 10.1002/adfm.202314565 is OK
- 10.1063/1.4812323 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 7 months ago

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

berquist commented 7 months ago

Review checklist for @berquist

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

RMeli commented 7 months ago

Hello @berquist and @srmnitc, how are you? How is the review coming along?

@srmnitc please generate your review checklist with

@editorialbot generate my checklist

when you have time, so that you can track progress.

srmnitc commented 7 months ago

Review checklist for @srmnitc

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

srmnitc commented 7 months ago

@RMeli I have now finished my review. Lobsterpy is a well-designed, and maintained package which I believe would be useful for the community. It has already been employed in a number of studies. The code and documentation are in very good shape, and it was really easy to get started with the tool. I had only some minor issues, which @JaGeo and team fixed very quickly. There are only minor corrections to the paper remaining. Overall, I recommend the publication of this package. Thanks @RMeli for inviting me to review, and @JaGeo for this nice software.

JaGeo commented 7 months ago

Thank you very much, @srmnitc (I mixed up the handles first, sorry!) We are, of course, happy to include corrections to our paper as well.

RMeli commented 7 months ago

Thank you @srmnitc for all the hard work!

I had only some minor issues, which @JaGeo and team fixed very quickly.

I've noticed it, thanks @JaGeo for the very prompt replies to the reviewer queries and comments.

There are only minor corrections to the paper remaining.

Did you suggest these minor corrections anywhere? I only see https://github.com/JaGeo/LobsterPy/issues/225, are you referring just to those two suggestions?

srmnitc commented 7 months ago

Thank you @srmnitc for all the hard work!

I had only some minor issues, which @JaGeo and team fixed very quickly.

I've noticed it, thanks @JaGeo for the very prompt replies to the reviewer queries and comments.

There are only minor corrections to the paper remaining.

Did you suggest these minor corrections anywhere? I only see JaGeo/LobsterPy#225, are you referring just to those two suggestions?

Yes, correct, just the two.

srmnitc commented 7 months ago

@editorialbot generate pdf

editorialbot commented 7 months ago

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

srmnitc commented 7 months ago

Thank you @srmnitc for all the hard work!

I had only some minor issues, which @JaGeo and team fixed very quickly.

I've noticed it, thanks @JaGeo for the very prompt replies to the reviewer queries and comments.

There are only minor corrections to the paper remaining.

Did you suggest these minor corrections anywhere? I only see JaGeo/LobsterPy#225, are you referring just to those two suggestions?

Once https://github.com/JaGeo/LobsterPy/pull/226 is merged, all my comments would be addressed. All comments addressed :)

JaGeo commented 7 months ago

We (@naik-aakash and I) have addressed the comments in https://github.com/JaGeo/LobsterPy/pull/226, @RMeli .

naik-aakash commented 7 months ago

@editorialbot generate pdf

editorialbot commented 7 months ago

:warning: An error happened when generating the pdf.

naik-aakash commented 7 months ago

@editorialbot generate pdf

editorialbot commented 7 months ago

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

RMeli commented 7 months ago

Hello @berquist, how are you? How is the review coming along? Please let me know if you have any questions or blockers.

berquist commented 7 months ago

Hi Rocco, sorry I've been unresponsive, I should be done in a day or two unless there are issues or PRs I need to file against the main project repo.

RMeli commented 7 months ago

No pressure @berquist! I'm just checking in periodically to make sure everything is alright. =)

JaGeo commented 7 months ago

There is now a slight delay from our side. We hope to include an additional suggestion from @berquist for our tutorial in the next few days. See here https://github.com/JaGeo/LobsterPy/pull/232

JaGeo commented 6 months ago

The comments on the tutorial from you, @berquist, have been addressed. Please let us know if you have further suggestions.

(Cc @RMeli )

berquist commented 6 months ago

Review

I've finished reviewing the code, documentation, and the paper, and am happy to recommend LobsterPy for submission to JOSS. My background is in molecular electronic structure, not materials or periodic systems, but there is clearly a substantial amount of work done here. I also get the impression that a lot of this work is hiding in atomate2.

The sections that follow are not required for acceptance, but I think will make the code friendlier for both users and developers, even if those developers are not external contributors.

Documentation

Reading https://jageo.github.io/LobsterPy/reference/cli_subcommands/createinputs.html there is the statement

It works only with PBE POTCARs.

that doesn't appear when running lobsterpy create-inputs --help. More importantly, no justification is given for the statement, but I think I found it in https://doi.org/10.1002/cplu.202200123

For the time being, two basis sets (Koga and pbeVASPfit2015) can be used to cover the whole periodic table, although only pbeVASPfit2015 includes additional orbitals important to the solid state (e. g., 2p in metallic Be[21]).

I would at least reproduce this comment in the help string, and ideally put more of the reasoning somewhere in the documentation, perhaps under a general "limitations" section.

For creating inputs, it would be good to have a simple example for a common case: it wasn't clear to me if the test data used by test_cli.py is there only for edge cases or for the common case as well.

Developer experience

Some suggestions on the developer experience:

I am happy to open PRs for any of these if you wish, though adding the CI service is purely an admin action.

RMeli commented 6 months ago

Thank you @berquist for the review and recommendation!

The sections that follow are not required for acceptance, but I think will make the code friendlier for both users and developers

@JaGeo (and team), would you be able to look into the additional suggestions and apply them where you see appropriate?

JaGeo commented 6 months ago

@RMeli , yes, sure, we will try to go ahead with this starting from next Monday. 🙂

@berquist , thank you for the review. All of the suggestions are extremely helpful! And we are surely happy about PRs.

JaGeo commented 6 months ago

Thanks again to @berquist for all the helpful suggestions. We have now addressed the comments in different PRs. @berquist has kindly also addressed some of the comments (thanks!)

Documentation

Reading https://jageo.github.io/LobsterPy/reference/cli_subcommands/createinputs.html there is the statement

It works only with PBE POTCARs.

that doesn't appear when running lobsterpy create-inputs --help. More importantly, no justification is given for the statement, but I think I found it in https://doi.org/10.1002/cplu.202200123

For the time being, two basis sets (Koga and pbeVASPfit2015) can be used to cover the whole periodic table, although only pbeVASPfit2015 includes additional orbitals important to the solid state (e. g., 2p in metallic Be[21]).

I would at least reproduce this comment in the help string, and ideally put more of the reasoning somewhere in the documentation, perhaps under a general "limitations" section.

For creating inputs, it would be good to have a simple example for a common case: it wasn't clear to me if the test data used by test_cli.py is there only for edge cases or for the common case as well.

We have now extended the current REAMDE.md to also cover this limitation of the cli functionality to create inputs. Furthermore, we added one example to the example folder including REAMDE.md. (see https://github.com/JaGeo/LobsterPy/pull/244)

Developer experience

Some suggestions on the developer experience:

  • The unit tests take a while to run (8 minutes in serial, 1.5 minutes with -n 8 --dist worksteal on a 16 core machine). The provided suggestion to use xdist is a good one. If you find that developers complain still, consider using pytest markers to group tests by time.
  • The top-level requirements.txt doesn't seem to be used anywhere. Since it is already covered properly by the pyproject.toml, consider removing it.
  • Consider removing black from the pre-commit config and using - id: ruff-format since the config is already in pyproject.toml. Similarly, the top-level pylintrc is confusing since it isn't part of pre-commit. Consolidating around tools and cleaning up their configurations will make your lives easier even if it doesn't affect users.

These first comments have been kindly addressed by @berquist . Thus, I hope this is all fine now.

  • I strongly recommend adding https://pre-commit.ci/ to the GitHub workflow so that all the pre-commit rules can be enforced for each PR even if someone forgets to install the hook locally. (I see configuration for it exists, so someone has considered this before, but it's not turned on.)

I have added the pre-commit.ci to the github workflow and have added the corresponding badge on the README.md (https://github.com/JaGeo/LobsterPy/pull/240)

@berquist , we hope this is now all fine from your side. If so, we would release a new version of the code. (cc @RMeli )

berquist commented 6 months ago

Yes, all is good.

RMeli commented 6 months ago

Thank you very much @berquist and @srmnitc for the timely and in-depth review, it's really appreciated! I see you checklists are now complete.

Thanks also to @JaGeo (and team) for engaging so well with the reviewers.

I'll have a good final look at the submission too, and then you can create a new release.

RMeli commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

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

RMeli commented 6 months ago

@JaGeo I see you have the repository already mirrored on Zenodo. Would it make sense to add the name of the software to the Zenodo title? Something like LobsterPy: Automated Bonding Analysis with Crystal Orbital Hamilton Populations?

RMeli commented 6 months ago

@JaGeo Is affiliation (1) correct? "Department Materials Chemistry" sounds a bit odd to me (but I'm not a native English speaker). I see that on the BAM website it is called "Materials Chemistry department"? Or maybe "Department 6: Materials Chemistry" would be more appropriate? If it's correct as it is, please discard my comment.

JaGeo commented 6 months ago

@JaGeo Is affiliation (1) correct? "Department Materials Chemistry" sounds a bit odd to me (but I'm not a native English speaker). I see that on the BAM website it is called "Materials Chemistry department"? Or maybe "Department 6: Materials Chemistry" would be more appropriate? If it's correct as it is, please discard my comment.

We can surely replace it with Materials Chemistry Department! Thanks for the suggestion!

JaGeo commented 6 months ago

@JaGeo I see you have the repository already mirrored on Zenodo. Would it make sense to add the name of the software to the Zenodo title? Something like LobsterPy: Automated Bonding Analysis with Crystal Orbital Hamilton Populations?

Yes, I can change this.

JaGeo commented 6 months ago

@RMeli :

Zenodo: https://zenodo.org/records/10631237 Would this be fine? I would repeat this change for our next release as well.

Affiliation: I changed the affiliation as suggested: https://github.com/JaGeo/LobsterPy/blob/main/paper/paper.md

JaGeo commented 6 months ago

@editorialbot generate pdf

editorialbot commented 6 months ago

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

RMeli commented 6 months ago

Zenodo: https://zenodo.org/records/10631237 Would this be fine? I would repeat this change for our next release as well.

Looks great, I think this will be much easier to find.

RMeli commented 6 months ago

@JaGeo I see that the author list on Zenodo is also different from the author list of the paper. The JOSS editorial guide states that

Check the archive deposit has the correct metadata (title and author list), and request the author edit it if it doesn’t match the paper.

This is my first time editing, I'll ask internally if this is an hard rule but as a minimum the authors in the paper should be all included in the Zenodo citation (I think); the other way around I don't think should hold. Will the missing authors be added with the next release or do you need to change this manually?

JaGeo commented 6 months ago

@JaGeo I see that the author list on Zenodo is also different from the author list of the paper. The JOSS editorial guide states that

Check the archive deposit has the correct metadata (title and author list), and request the author edit it if it doesn’t match the paper.

This is my first time editing, I'll ask internally if this is an hard rule but as a minimum the authors in the paper should be all included in the Zenodo citation (I think); the other way around I don't think should hold. Will the missing authors be added with the next release or do you need to change this manually?

I will change this manually. The problem comes from the CITATION.cff file in the repo. Zenodo uses it as a basis for the citation. I am happy to do this correctly for the next release!

RMeli commented 6 months ago

I asked internally how it works. I hope that the Zenodo list can be a superset (i.e. all contributors) of the JOSS authors (main/core contributors). I'll let you know ASAP how to proceed.

RMeli commented 6 months ago

Everything else looks good. =)

JaGeo commented 6 months ago

Great, @RMeli . We just had this additional question, if we could acknowledge the reviewers in the paper.

RMeli commented 6 months ago

@JaGeo yes, you can acknowledge anyone you want, including the reviewers.

JaGeo commented 6 months ago

@RMeli As a part of this PR that will also fix some tiny issues with the README and our code coverage determination, we have added that acknowledgment now: https://github.com/JaGeo/LobsterPy/pull/246

JaGeo commented 6 months ago

@editorialbot generate pdf

JaGeo commented 6 months ago

I will release a new version of the code and the corresponding archive on zenodo as soon as you let me know about the requirements for the authors, @RMeli