[REVIEW]: effmass: An effective mass package

whedon commented 6 years ago

Submitting author: @lucydot (Lucy Whalley) Repository: https://github.com/lucydot/effmass Version: v1.0.b1 Editor: @katyhuff Reviewers: @ajjackson, @bocklund, @mkhorton Archive: 10.5281/zenodo.1338919

Status

Status badge code:

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

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

@ajjackson, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @katyhuff know.

Review checklist for @bocklund

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

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] Version: Does the release version given match the GitHub release (v1.0.b1)?
[x] Authorship: Has the submitting author (@lucydot) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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 function 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] Authors: Does the paper.md file include a list of authors with their affiliations?
[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] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @ajjackson

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

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] Version: Does the release version given match the GitHub release (v1.0.b1)?
[x] Authorship: Has the submitting author (@lucydot) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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 function 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] Authors: Does the paper.md file include a list of authors with their affiliations?
[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] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @mkhorton

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.
- I have an older (> 1 year) affiliation with the author's department/institution, but this does not seem to constitute a conflict of interest according to the JOSS conflict of interest policy.

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] Version: Does the release version given match the GitHub release (v1.0.b1)?
[x] Authorship: Has the submitting author (@lucydot) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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 function 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] Authors: Does the paper.md file include a list of authors with their affiliations?
[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] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
- ~~paper.md has references in the text, but is lacking the actual references at the end of the document(?), I think it just has to be re-built~~

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @ajjackson it looks like you're currently assigned as the reviewer for this paper :tada:.

: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

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

bocklund commented 6 years ago

Here is my first pass:

General checks

Version: The latest PyPI version is 0.1, behind 0.1.1 and 1.0.b1, but it seems like the the author is working on CI-backed releases. The released version does not work with the current examples on GitHub.

Functionality

Functionality: Issue https://github.com/lucydot/effmass/issues/1 was opened. Once closed this can be checked off.

Documentation

A statement of need: effmass depends on pymatgen through vasppy. @lucydot, will you consider comparing effmass to existing software like pymatgen or ASE (what it provides that those do not) in the documentation?
Community guidelines: There are not very clear guidelines for contributing or a code of conduct (item 1). Support seems to implicitly offered through GitHub, but maybe this is not the author's intention (item 3).

Other comments

Since this already depends on pymatgen, is it possible that effmass can support parsing electronic structures from other software?

lucydot commented 6 years ago

Hi @bocklund and thank you for taking the time to look over effmass

General checks

version: The latest release contains the changes below and I have uploaded this to PyPI; it should install with pip install effmass=1.0b3

Functionality

Functionality: I have fixed the bug and closed the issue: please see lucydot/effmass#1 for details

Documentation

A statement of need: Thank you for this idea - I have added a paragraph to the documentation here.
Community guidelines: I had not thought of including a code of conduct but I think it is a great idea. I have added CODE_OF_CONDUCT.md and link to it from the README.md

Other comments

I agree that it would be nice for effmass to support parsing of electronic structures from other software, and it should be relatively easy to implement. I aim to set time aside for a future release, and have suggested it as a possible contribution in the README.md.

bocklund commented 6 years ago

@lucydot, thanks for your response. Looks good to me.

cc @katyhuff

katyhuff commented 6 years ago

Excellent! Thank you for your review @bocklund .

@ajjackson are you still interested in completing a review of this work?

@mkhorton -- it looks like you've begun your review. How is the remainder of the review process going?

ajjackson commented 6 years ago

Yes, I have been looking at it today!

As Lucy mentioned in the Pre-review thread, we were members of the same research group a little over two years ago but we never published together. I was not aware of the existence of this code until asked to review it.

I will check off what I can now and then try out the code on some fresh data.

mkhorton commented 6 years ago

@katyhuff I had trouble running the tests, but wanted to investigate further before commenting (it might be my own issue), otherwise looking good.

I do echo @bocklund's comment though that since pymatgen is already a requirement of this package, it would be good to support pymatgen's own native pymatgen.io.vasp classes. I don't think this has any impact on the review however (as a disclaimer, I have contributed to pymatgen myself).

One point I did want to make, is that according to JOSS' stated policy on novel software:

Submissions that implement solutions already solved in other software packages are accepted into JOSS provided that they meet the criteria listed above and cite prior similar work.

There are a few other related packages that also try to calculate effective mass. I think effmass goes further than these other tools do, but it would be good to mention them in the paper.md:

emc sumo

ajjackson commented 6 years ago

As a developer of Sumo (which is also going through JOSS review), I am comfortable with the amount of overlap between effmass and Sumo. Effmass is more flexible for effective mass fitting and concentrates on providing a toolkit for analysis. Sumo is a suite of command-line tools; it is more opinionated and "fire-and-forget", with a less Python-adept target audience.

ajjackson commented 6 years ago

In general effmass seems to be packaged and documented very nicely. I need to take a bit more time to play with the actual functionality. Here are some comments in the meantime:

General checks

version: the PyPI/beta/source versioning was a touch confusing but as far as I can see the packaging all works as it should and I was able to get the 1.01 beta versions easily enough. I think this is ok.

Documentation

Statement of need: The summary of unique features is helpful, a very useful thing to have when people are looking for tools. I would like to see a more explicit statement somewhere of the target audience (i.e. theoretical solid-state physics/chemistry community). The review guidelines imply that this should be in both the paper and other docs.
Installation instructions:
- The Python package management works well and I don't think a separate requirements list is essential. I do think it is worth mentioning that Numpy/Matplotlib are used as these dependencies are a) quite large b) sometimes best installed with a different package manager.
- Numpy is listed as a dependency in setup.py and requirements.txt, but SciPy is not. Scipy is imported in analysis.py so seems to be a required module. It is one of Pymatgen's many dependencies so the installers work, but this seems inconsistent when other packages installed by Pymatgen are listed explicity.
- requirements.txt is missing Pymatgen. I assume this is related to making readthedocs work, as we ran into exactly this issue on Sumo. Probably the best solution is to name it requirements_rtd or something so that people don't try to use it with virtualenv. EDIT: actually it was Phonopy we had that problem with. Presumably Pymatgen is being installed as a Vasppy dependency?
Community guidelines: The guidelines for contributors are good. The guidelines for bugs/support are minimal but do the job.

@bocklund and @mkhorton have both noted the Pymatgen dependency as a basis for further development. Poking around though, I'm not sure that effmass actually uses Pymatgen at all? Perhaps I've missed something, but as far as I can see it's just a side-effect of installing Vasppy?

mkhorton commented 6 years ago

@ajjackson yeah, it's a little weird -- I'm not actually familiar with vasppy, but it looks like pymatgen is a dependency of vasppy but it doesn't actually use pymatgen much. In particular, vasppy itself doesn't seem to build on pymatgen's VASP classes, but rather has its own VASP classes, which means building in pymatgen support to effmass would indeed be a bit trickier. (Hello by the way, it's a small world -- I didn't realise you were a developer of sumo!)

lucydot commented 6 years ago

Thank you @ajjackson and @mkhorton for your suggestions.

Overlap with other packages

I've added a Related packages section to the paper.md. This has also been added as a new page in RTD, and a link to this page has been added to the README.

Target audience

I have explicitly stated the target audience as part of the Related packages documentation (see above).

Installation instructions

I have included explicit mention of the scipy, numpy and matplotlib packages as suggested.

Dependencies: scipy

Thank-you for pointing out I had missed listing scipy as a dependency in in requirements.txt and setup.py - this was an oversight and it is now added.

Dependancies: pymatgen

I haven't included pymatgen as effmass does not make use of this (it is only installed as it is a dependency of vasppy). To enable easy extension to other electronic structure code outputs, there is definitely an argument for losing the vasppy dependency and using pymatgen instead, though this would involve re-writing a significant part of the code.

katyhuff commented 6 years ago

Thank you @lucydot . It looks like you've addressed the reviewer comments. Let's see if they can recommend publication.

@mkhorton & @ajjackson : Thank you for your reviews. Do you have any remaining comments? Are you ready to make a final recommendation for acceptance based on this response, or would you like more time to explore the package?

mkhorton commented 6 years ago

I've updated my checklist. Ideally, I'd like some further time to play with this to check off the remaining items, but I'm happy to defer to the other referees if they're happy to accept now -- it certainly seems like a very nicely structured package and will be a valuable addition to the community.

katyhuff commented 6 years ago

Thanks @mkhorton . Feel free to explore a bit more. I'll check back in next week.

ajjackson commented 6 years ago

I have worked through the Jupyter tutorial and was able to follow it and reproduce the plots without any trouble. It gives an applied overview of features; the comparison with DOS filling is a nice touch.

Issue raised over broken link to data
I can reproduce step [5] fine but it's not clear how it should be interpreted. The docstring says "Plots bandstructure with the DFT-calculated points for each Segment instance overlaid." but if I had to guess what the vector labels are I'd say they are directions? It would be really helpful if these labels also included the segment indices.
I have raised a feature request here which I think would improve usability. This should not be considered a condition of acceptance, it's a "nice to have".

I'd like to throw some of my own data into the mix before signing off

lucydot commented 6 years ago

Thanks for the feedback @ajjackson:

I have fixed the link and closed the issue.
The labels now include the segments argument index. See the notebook for the example output - have I interpreted your suggestion correctly? The labels tended to overlap, so I have used the adjustText package to avoid this, and I think the output is much easier to read now. The function docstring and notebook have been updated so that it is clear what the labels correspond to.
I have implemented the feature request: https://github.com/lucydot/effmass/issues/3 - thanks for the suggestion, it echos a similar (verbal) suggestion from @Guaxixi .

ajjackson commented 6 years ago

Review updates:

Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)? In general effmass is following best practice in the use of Python docstrings and automatically-generated documentation. I have checked this off as satisfactory, with the following observations:

I found a minor error in a docstring, raised as an issue here: https://github.com/lucydot/effmass/issues/6
It would be nice to have a bit more description in the API documentation re: what the modules are for. My own code documentation is also sparse in this respect... something to thing about for the future!

References It's difficult to check the references since the paper was updated as the PDF manuscript hasn't been rebuilt. I don't see a formal academic citation for the Seebeck effective mass paper (is that deliberate to indicate that it isn't used in this package?), or for pymatgen (https://doi.org/10.1016/j.commatsci.2012.10.028).

Functionality My attempt to test with independent data was foiled by some issues with partial occupancies; I will have another go before signing off if that's ok. https://github.com/lucydot/effmass/issues/5

katyhuff commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

katyhuff commented 6 years ago

@ajjackson please see built pdf below (to see what whedon can do, you can execute @whedon commands at any time)

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

katyhuff commented 6 years ago

@ajjackson regarding your concern about api docs, the readme for the repo links to these API docs which seem quite sufficient to me : https://effmass.readthedocs.io/en/latest/

ajjackson commented 6 years ago

I agree they are sufficient, it's not a big deal :-) I was referring to pages such as https://effmass.readthedocs.io/en/latest/API%20documentation.html which currently contains links without descriptions. This is very common in automatically generated documentation.

katyhuff commented 6 years ago

Ah, I misunderstood. Your comment initially made me think you couldn't find your way to the API pages. (apologies)

Yes, I agree this is the default sphinx/readthedocs behavior. More might be better on that page, perhaps. But in this case, I think the docs are strong and it's not necessary if the reader has the patience to click through the links.

mkhorton commented 6 years ago

@lucydot running python setup.py test fails with:

  File ".../effmass/tests/test_inputs.py", line 10, in <module>
    (pytest.lazy_fixture('MAPI_settings_object'), 0.025, 0.25),
AttributeError: module 'pytest' has no attribute 'lazy_fixture'

Your tests are working correctly with python -m pytest as recommended by your README, so this is not a bug, but I thought I'd bring this to your attention since it might trip people up.

ajjackson commented 6 years ago

I've raised an issue here about spin-polarised calculations, happy to tick the last box once that's resolved!

I intend to use this package. It provides convenient comparison of different fitting methods, which is a good sanity check, and implements a superior weighted method for effective-mass determination. The Kane model implementation provides a useful way of quantifying non-parabolicity. The packaging follows best practices, and decent API documentation is accompanied by a useful tutorial.

lucydot commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

lucydot commented 6 years ago

Thanks for the feedback @mkhorton and @ajjackson, it is really useful. @ajjackson I am chuffed you are planning to make use of it :smiley:

@mkhorton thanks for pointing out the possible testing pitfall, I've raised it as an issue on the repo.

Functionality:

I have looked at the issues raised @ajjackson (here and here), let me know whether these solve the problem and if so I will close them.

Documentation:

I have fixed the issue regarding out of date docstring here.
I have added some module level docstrings to the API documentation here.

References

I have included academic references for pymatgen. I will add a reference for sumo once it has a Zenodo / JOSS DOI.
I haven't included the Seebeck reference because, as you say, it isn't implemented in effmass. I do include a link to a paper which contains a definition and further references.

katyhuff commented 6 years ago

This looks like a very comprehensive response @lucydot . I think that these changes are sufficient and hopefully @ajjackson will confirm. Assuming this is confirmed, I think this is ready to accept. I know you intend to include a reference to the sumo JOSS paper, but I don't think the review process for sumo should delay the review process for this work. If a reference to the sumo repository can be added, perhaps that is a good middle ground.

In any case, as soon as we get confirmation from @ajjackson that these issues satisfy the concerns, this will be ready to accept. At that point, @lucydot , could you please make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

katyhuff commented 6 years ago

It looks like there are still two open issues that you're communicating in (here and here. Perhaps one of you (@lucydot @ajjackson) when we're ready to move forward with the submission.

lucydot commented 6 years ago

Yes, I'd like to sort out the discrepancy with the sumo package in issue #7 - it looks like effmass is not handling spin-polarised data properly.

ajjackson commented 6 years ago

Yes, Issue 5 (partial occupancy) is fine now. Just waiting on issue 7 which affects the calculated values.

lucydot commented 6 years ago

Latest commit to master fixes issue 7- thank-you for finding this bug @ajjackson, it was an important one!

katyhuff commented 6 years ago

Fantastic. At this point, @lucydot , could you please make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission!

lucydot commented 6 years ago

Issue 7 is not quite closed (but very close!), when it is I will archive and update this thread.

katyhuff commented 6 years ago

Ah, I misunderstood the previous comment. Thanks.

ajjackson commented 6 years ago

Issue 7 looks good now, I have ticked off "functionality".

katyhuff commented 6 years ago

Excellent. @lucydot when you make archive of the reviewed software in Zenodo/figshare/other service, please just update this thread with the DOI of the archive and we'll move forward with acceptance! (You may want to be sure to double check the paper, all minor details, etc. before creating the archive).

katyhuff commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

lucydot commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

lucydot commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

openjournals / joss-reviews