[REVIEW]: pyscses - a python space charge site explicit solver

whedon commented 5 years ago

Submitting author: @georgiewellock (Georgina Wellock) Repository: https://github.com/bjmorgan/pyscses Version: v1.0.0 Editor: @labarba Reviewer: @ncclementi, @vyasr Archive: 10.5281/zenodo.2599955

Status

Status badge code:

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

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

@ncclementi & @vyasr, 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 @labarba know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @ncclementi

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?
[ ] Version: v1.0.0
[x] Authorship: Has the submitting author (@georgiewellock) 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 @vyasr

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: v1.0.0
[x] Authorship: Has the submitting author (@georgiewellock) 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)?

whedon commented 5 years ago

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

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

labarba commented 5 years ago

👋 @georgiewellock Please check your references list, aided by the bot's suggestions in the pre-review issue: https://github.com/openjournals/joss-reviews/issues/1163#issuecomment-457926514

labarba commented 5 years ago

@ncclementi, @vyasr — Thank you for agreeing to review for JOSS! This is where the action happens: work your way through the review checklist, feel free to ask questions or post comments here, and also open issues in the submission repository as needed. Godspeed!

georgiewellock commented 5 years ago

👋 @georgiewellock Please check your references list, aided by the bot's suggestions in the pre-review issue: #1163 (comment)

I have updated the two references that have DOIs, but the other two do not. Frenkel (1946) does not appear to have been published digitally and the other is an unpublished paper we hope to have come out alongside the JOSS publication. Latest commit: https://github.com/bjmorgan/pyscses/commit/21a2355cbf22d06e8e23829445d8cd525f19f11b

labarba commented 5 years ago

In regards to the unpublished paper from the same authors, I want to encourage you to post the pre-print in a repository like arXiv, ChemRxiv or similar. A citation to a non-public document is discouraged.

vyasr commented 5 years ago

Nice work on the project! Here are some initial requests from my first pass of review, most of them shouldn't be too difficult.

[x] Remove the extraneous test_notebook folder in the root of the repo, as far as I can tell it's not being used any longer.
[x] Currently the unit test structure is a bit confusing. The Python files in the tests/ directory fail because pyscses is still referred to as project, but even when I modify that naming issue I get other issues suggesting that those tests are using an old API. I'd remove those files, since they look like old versions of the files in tests/unit_tests.
[x] The tests in tests/unit_tests mostly work, except test_grid.py fails from trying to import volume_from_grid. It would be good to fix that, and then move these files directly into the tests directory since that's probably the more expected structure.
[x] The userguide looks pretty good overall, you should fill out or get rid of the README file there though. Also, you should look into nbsphinx, which will allow you to post the userguide as part of your RTD documentation. People shouldn't have to clone the repository to access that guide, since it's probably the more useful part of the documentation than the API. Let me know if you need examples of how this is done.
[x] There's currently something wrong with the API documentation on ReadTheDocs. I'm not sure if it's an issue with RTD itself or with your build, but currently almost nothing actually shows up there. Please try and fix that, and let us know if there's some reason you can't get it working.
[x] The package appears to be built on a version of sympy that's many years old. The mpmath submodule of sympy was removed as of 0.7.6, and the current version is 1.3, so you'll need to add that as a separate dependency to make sure things still work.
[x] Make sure the test notebooks actually run successfully; I ran into a couple of (easy-to-fix) issues such as needing to install pandas (that should just be documented at the top of the notebook), having to make the directory for storing outputs, etc.

bjmorgan commented 5 years ago

In regards to the unpublished paper from the same authors, I want to encourage you to post the pre-print in a repository like arXiv, ChemRxiv or similar. A citation to a non-public document is discouraged.

We have removed this unpublished reference (https://github.com/bjmorgan/pyscses/commit/65f5e940961175d1d029d41784eccb05ccedb67e). If it is ready for publishing as a preprint before this review is complete we will add it back at that stage.

bjmorgan commented 5 years ago

API documentation appears to be fixed after the latest commit (https://github.com/bjmorgan/pyscses/commit/1abbde059886511f82d6cba1056916a31e97e2a7).

e.g. https://pyscses.readthedocs.io/en/latest/pyscses.html

ncclementi commented 5 years ago

Nice job people, here are my comments, requests and questions so far on each part that I'm suppose to review. I haven't been able to get to the tests part yet, but I saw @vyasr made some comments already.

I divided my requests, questions and comments by following the structure that is given as for the review, you can find it on the first comment.

General comments, requests and questions regarding:

Version: Does the release version given match the GitHub release (v 0.9.3.2)?

It matches now but if you modify things after the review, you will want to do a release and in that it case it won't match, how do we deal with this? @labarba are there any suggestion regarding this?

Authorship: Has the submitting author (@georgiewellock) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

According to the joss reviewer guidelines, I need to check weather the submitting author has made "substantial contribution" to the submitted software (as determined by the commit history). https://joss.readthedocs.io/en/latest/review_criteria.html

The submission was made by the username @georgiewellock but this account has no commits in the repository contributors section https://github.com/bjmorgan/pyscses/graphs/contributors

Can you explain how you meet the criteria?

Functionality

Installation: Does installation proceed as outlined in the documentation?

I tried to installed via pip and had no problem until I tried to run and had problem with mpmath dependency.

Setting up the notebook to run calculations:

ImportError: cannot import name 'mpmath' @vysar already point this out. You need to add this a separate dependency to make sure things work. I've saw this modified as import mpmath but after installing mpath separately and pulling the latests changes from the repo I tried to install pyscses following the instructions but I still can't run the setup notebook. This is because all the installation instructions rely on pip and pip doesn't have the latest version.

You should add an option of installing it directly using the setup.py and that way it works.

Also pandas seems to be needed to run the notebooks too, but it's not listed as a dependency, it should be added.

Lastly, the software is run by using Jupyter notebooks, even though this is mentioned later in the documentation, might be useful to mention it at the installation level.

Functionality: Have the functional claims of the software been confirmed?

To run the examples and tests I need certain notebooks that are in the repo that I can't download all together unless I clone the repo.

Is there a better way to do this? you should put a note or description on how to get the examples and tests. Since we can't run the notebooks from github directly it would be useful to add a comment on where in the repo are located instead of putting "definitions and example code can be found in the userguide." where userguide is a specific link.

Make sure that all your notebooks run clean and it would be useful to have information on the time it takes to run some of the examples.

I open an issue on the repo so you can give me an estimate on the timing and maybe on the future add it to the documentation, since I've been running Example 1 for about 15 min and I don't know if this is the expected behavior.

Running notebook: Cell number 6:


NameError                                 Traceback (most recent call last)
<ipython-input-6-67db249c57f8> in <module>()
  4     for t in temp:
  5 
----> 6         defect_species = { l : DefectSpecies( l, v, m, mob ) for l, v, m, mob in zip( defect_labels, valence, m, mobilities ) }
  7 
  8         all_sites = Set_of_Sites.set_of_sites_from_input_data( data, [grid_x_min, grid_x_max], defect_species, site_charges, core_models, t )

NameError: name 'mobilities' is not defined



**Performance**: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

There are no claims of performance so I checked the item off.

### Documentation

**A statement of need**: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

I think that the summary on the readme doesn't clearly say what problems the software is 
designed to solve. There is a sentence that opens the paragraph:

"pyscses is a Python module that implements a site-explicit, one-dimensional Poisson-Boltzmann solver, used for modelling ionic space charge properties in solid materials."

Is think you should add more that explains the target problems to general audience and why this problems are important.  

**Installation instructions**: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

Read comments on Installation made in the Functionality section. 

**Example usage**: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

There are examples on how to use it. Haven't been able to run them yet. I need 
a better idea on the time they take to run (explained in Functionality section above)

**Functionality documentation**: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

There is API documentation in the webpage for each class, and the documentation of each me
method.  

**Automated tests**: Are there automated tests or manual steps described so that the function of the software can be verified?

Haven't get to this part yet, but I see @vysar made some comments on it so I will work on this once his suggestions are included. 

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

There are no guidelines on how to contribute to the software, report issues or problems with the software or how to seek support. 

### Software paper

Comments following the checklist:

Keeping in mind that according to the authors guidelines "A summary describing the high-level functionality and purpose of the software for a diverse, non-specialist audience"

**A statement of need**: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

I'm not sure if I understand where is the statement of need presented in the software and who is the target audience. 

At the beginning of the the paper you mention what `pyscses` is used for:
"modelling ionic space-charge regions in crystalline solids." 

Can you break down this for a more general audience? 

The following two paragraphs intend to explain more about the problem but it is hard to understand, at least for someone that is not from the area. I get a bit lost due to the specific vocabulary and some of the sentences are too long. For example, the sentence that starts with "For example, ..." in the second paragraph goes for five lines. 

and at the end of the second paragraph you say:

 "In the case of solid electrolytes, understanding space-charge formation at grain boundaries and interfaces is a key challenge in developing a theoretical description of the role of crystalline microstructure on macroscopic ion transport (ionic conductivities) [@KimEtAl_PhysChemChemPhys2003]." 

This tells me why "understanding space-charge formation..." is important but not what is the purpose of the software. I'm missing the connection between the problem, its importance and the software itself. 

**References**:

Regarding the references, you have an "in preparation" paper but it is cited in the numerical model. Are you using `pyscses` for this research, it is not clear to me? If so, it is important to mention it. According to authors guidelines "Mentions (if applicable) of any ongoing research projects using the software or recent scholarly publications enabled by it".

#### General comments regarding the paper: 

If we go back to the documentation on [Submitting a paper to JOSS](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain)

I think the paper is too long. You might want to review some of the examples of published papers, or even though the one in the documentation. 

There are multiple parts where you explain some of the software functionality and workflow (End of numerical model section and "Typical workflow section"), and this should not be included. According to the review criteria:

"The paper should not include software documentation such as API functionality,
as this should be outlined in the software documentation."
https://joss.readthedocs.io/en/latest/review_criteria.html#review-items

*Numerical model section*

Regarding this section, I don't think is necessary to have it on the paper since most of it is explained with the same words in the `Theory.ipynb` notebook and example notebooks.

*Calculated properties, approximations and limitations sections*

I think these shouldn't be separate sections. I would suggest to rephrase all the information in this sections, add it to the summary and remove the math. 

**Remember** that the core of the paper should, according to the guidelines, contain :

- A summary describing the high-level functionality and purpose of the software for a diverse, non-specialist audience
- A clear statement of need that illustrates the purpose of the software

Acknowledgments:

There is a grant code missing: (EPSRC grant code?).

labarba commented 5 years ago

@ncclementi — You say that you opened an issue on the submission repo. Can you add a link to that issue? The cross-links are nice to have on the Review thread. Thanks!

Regarding the software version, after changes stemming from the review, the authors will have to up the version, and we'll use the superpowers of our bot to update it (@whedon set v1.x as version).

ncclementi commented 5 years ago

Yes, sure! Here are the links of the issues I opened: https://github.com/bjmorgan/pyscses/issues/7 https://github.com/bjmorgan/pyscses/issues/10

bjmorgan commented 5 years ago

@ncclementi @vyasr Thank you both for your very thorough comments. We will begin working through addressing these points.

bjmorgan commented 5 years ago

@vyasr

Remove the extraneous test_notebook folder in the root of the repo, as far as I can tell it's not being used any longer.

Done in commit https://github.com/bjmorgan/pyscses/commit/969d7be62bd4fa8777383a274de2c6fa72d251ea

bjmorgan commented 5 years ago

@vyasr

[x] Currently the unit test structure is a bit confusing. The Python files in the tests/ directory fail because pyscses is still referred to as project, but even when I modify that naming issue I get other issues suggesting that those tests are using an old API. I'd remove those files, since they look like old versions of the files in tests/unit_tests.

[x] The tests in tests/unit_tests mostly work, except test_grid.py fails from trying to import volume_from_grid. It would be good to fix that, and then move these files directly into the tests directory since that's probably the more expected structure.

Both addressed in commit https://github.com/bjmorgan/pyscses/commit/969d7be62bd4fa8777383a274de2c6fa72d251ea

Although we only have very limited unit tests at the moment, we have also added continuous integration tests using Travis CI, to avoid these tests getting out of sync with the project API in the future.

bjmorgan commented 5 years ago

@vyasr

The package appears to be built on a version of sympy that's many years old. The mpmath submodule of sympy was removed as of 0.7.6, and the current version is 1.3, so you'll need to add that as a separate dependency to make sure things still work.

requirements.txt has been updated.

@ncclementi

I tried to installed via pip and had no problem until I tried to run and had problem with mpmath dependency.

Setting up the notebook to run calculations:

ImportError: cannot import name 'mpmath' @vysar already point this out. You need to add this a separate dependency to make sure things work. I've saw this modified as import mpmath but after installing mpath separately and pulling the latests changes from the repo I tried to install pyscses following the instructions but I still can't run the setup notebook. This is because all the installation instructions rely on pip and pip doesn't have the latest version.

You should add an option of installing it directly using the setup.py and that way it works.

See above re: updates to requirements.txt.

We have also updated the version on PyPI to let you test installation via pip.

The installation instructions in the top level README contain instructions for alternate installation directly from the GitHub source using setup.py:

Installation

The simplest way to install pyscses is to use pip to install from PyPI
pip install pyscses
Alternatively, you can download the latest release from GitHub, and install directly:
cd pyscses
pip install -e .
which installs an editable (-e) version of pyscses in your userspace.

Or clone the latest version from GitHub with
git clone git@github.com:bjmorgan/pyscses.git
and install the same way.
cd pyscses
pip install -e .

bjmorgan commented 5 years ago

@ncclementi

Authorship: Has the submitting author (@georgiewellock) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

According to the joss reviewer guidelines, I need to check weather the submitting author has made "substantial contribution" to the submitted software (as determined by the commit history). https://joss.readthedocs.io/en/latest/review_criteria.html

The submission was made by the username @georgiewellock but this account has no commits in the repository contributors section https://github.com/bjmorgan/pyscses/graphs/contributors Can you explain how you meet the criteria?

If you look at the full commit history at https://github.com/bjmorgan/pyscses/commits/master you can see the majority of commits are from glw33 who is @georgiewellock. It seems @georgiewellock's local git setup is using a different username than their GitHub account.

@labarba I'd like to flag a possible point of confusion in your reviewer / author instructions: Your guide for reviewers at present starts by saying that the commit history should be used to verify significant contributions, but then later says "active project direction and other forms of non-code contributions are [significant]". I can see that if someone has a substantial code contribution they should be considered as an author, but it is not necessarily clear at the moment that a significant commit history is not the only requirement for authorship.

bjmorgan commented 5 years ago

@ncclementi @vyasr Can you please add further comments and suggestions / those we have not yet addressed as issues on the main repository, so we can track which we have / have not addressed?

bjmorgan commented 5 years ago

@ncclementi

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

There are no guidelines on how to contribute to the software, report issues or problems with the software or how to seek support.

We have added guidelines for contributing / asking for support / reporting issues to the top level README: https://github.com/bjmorgan/pyscses/commit/8e36373c28ef79918970dd4b62890dec1ebdc2fb

ncclementi commented 5 years ago

@bjmorgan

Regarding installation: I will have to give it another try, because when I tried to installed (after the requirements were updated) I wasn't able to get it working doing: Or clone the latest version from GitHub with

git clone git@github.com:bjmorgan/pyscses.git

and install the same way.

cd pyscses
pip install -e .

The way it worked for me was by doing: python setup.py install

Regarding your request: "Can you please add further comments and suggestions / those we have not yet addressed as issues on the main repository, so we can track which we have / have not addressed?"

I'm not sure if I understand what you mean. I think went over all what is required to review on the big comment I made above, did I miss something?. I know that my path on the tests is pending but I will take a look at it soon. Are you referring to that, or something different?

bjmorgan commented 5 years ago

@ncclementi Can you please post your errors when you try to install using pip install -e .?

Regarding opening specific issues on the main repository: you have both provided comprehensive feedback on your first pass, and I want to make sure we don't miss any of the issues you have raised. It will probably be easier to track individual discussion threads in separate issues on the main repo than having things all mixed together here.

bjmorgan commented 5 years ago

@vyasr

The userguide looks pretty good overall, you should fill out or get rid of the README file there though. Also, you should look into nbsphinx, which will allow you to post the userguide as part of your RTD documentation. People shouldn't have to clone the repository to access that guide, since it's probably the more useful part of the documentation than the API. Let me know if you need examples of how this is done.

From my initial playing around, if we want to add these *.ipynb notebooks to the Sphinx RTD documentation, they need to be moved into the docs/source directory. I feel this makes them difficult to find if people want to access and run them as Jupyter notebooks. What are your thoughts on providing web-viewable versions using e.g. nbviewer --> https://nbviewer.jupyter.org/github/bjmorgan/pyscses/tree/master/userguides/notebooks/

labarba commented 5 years ago

you have both provided comprehensive feedback on your first pass, and I want to make sure we don't miss any of the issues you have raised. It will probably be easier to track individual discussion threads in separate issues on the main repo than having things all mixed together here.

@bjmorgan — Indeed, the reviewers have provided a detailed report of the issues you may need to address. For a few concrete ones, they opened a new issue on you software repository. With the rest, as it is posted here, I will request that you take the time to create a checklist of all the items that need addressing, and you go about checking them off (and open issues in your own repo as needed). I don't support requesting that the reviewers do this job for you.

ncclementi commented 5 years ago

@bjmorgan regarding the installation, I checked it again and installed form scratched and have no problem. We can call this solved, great job!

Note: Issue open regarding Running.ipynb https://github.com/bjmorgan/pyscses/issues/12

bjmorgan commented 5 years ago

@bjmorgan — Indeed, the reviewers have provided a detailed report of the issues you may need to address. For a few concrete ones, they opened a new issue on you software repository. With the rest, as it is posted here, I will request that you take the time to create a checklist of all the items that need addressing, and you go about checking them off (and open issues in your own repo as needed). I don't support requesting that the reviewers do this job for you.

@labarba We're happy to copy the more complex points to issues on the main repo to help track them.

ncclementi commented 5 years ago

Issue opened regarding presence of .ipynb_checkpoints https://github.com/bjmorgan/pyscses/issues/13

vyasr commented 5 years ago

My apologies, I've been traveling for the the last couple weeks and I lost track of this review. I see that you've addressed most of my comments from before, and you have an open question regarding the example notebooks. I'll give this another pass tomorrow and address the outstanding items. Sorry again for the delay, it looks like you've managed to continue making progress though, which is great!

ncclementi commented 5 years ago

In my case, I saw you addressed my comments on the software but it is still pending my comments on the paper itself. I will give another path to the rest of the points but I'd like the authors to address my comments on the paper.md.

vyasr commented 5 years ago

@bjmorgan regarding the notebooks, I see your point about wanting to keep them easily visible rather than hiding them in a docs folder. One solution would be to move the notebooks to docs/source/examples or so, and then symlink them to your tests directory (run ln -s docs/source/examples userguides in the root of the repository). Would that address your concern? I think that most users would be greatly aided by actually seeing these examples on RTD, since these days that's where most Python code hosts explanatory documentation in addition to API docs.

Having said that, I do think nbviewer is also a good option if you dislike my suggestion. If you decide to stick with nbviewer, I would definitely add that to both your top-level README.md and your sphinx index.rst.

Another alternative you may want to consider is mybinder. The service isn't perfect and does go down, so I would definitely only do that in addition to RTD/nbviewer, but it's really nice to be able to execute the notebooks directly online. That's just a suggestion though, not a hard request for this paper.

vyasr commented 5 years ago

The new contributing guidelines look good. I would suggest converting the "Issue Tracker" and "pull request" text into links.

vyasr commented 5 years ago

Regarding the contents of the paper, I'm mostly in agreement with @ncclementi. The paper is definitely too long at present, so I'll try to give pointers on where and how to cut as I make other comments.

I found the summary to be comprehensible, but I agree that it's probably a little too technical for a general, non-specialist audience unfamiliar with the behavior of defects and grain boundaries in crystal structures. I would probably summarize the first two paragraphs with something along the lines of: "Most real crystalline materials are characterized by defects, imperfections that violate the perfect symmetries that define the crystal. For example, in many crystals, two locally ordered regions that are oriented differently meet at what is called a grain boundary, where the two crystalline pieces fail to fit perfectly together. In ionic crystals, thermodynamic arguments show that charged point defects will migrate towards interfaces and grain boundaries, leading to the formation of surface space-charges, or regions of net charge. This segregation can impact various material properties, e.g. (INSERT JUST ONE EXAMPLE, SKIP THE REST). Therefore, understanding the distributions of point-charge defects is critical to understanding the properties of a material. (INSERT SENTENCE: WHY DEVELOP pyscses? ARE THERE NO OTHER GOOD TOOLS, NOTHING IN PYTHON, ETC)."

I don't quite agree with @ncclementi that you should remove this whole section, I think it's nice to have some of it. You can definitely combine most of the numerical model and calculated properties sections, and remove most of the math. I would probably drop the first two paragraphs and start the section with the paragraph on "Finding the equilibrium distribution". Keep the first equation there, then drop the rest and just assert that this leads to a system involving a modified PB equation that can be solved in a self-consistent fashion. You can point to your docs and userguides for more information. I actually like the bulleted list at the end of this section, and I would keep it since I don't consider that API level information. I'd remove the first two paragraphs of the typical workflow section, then just have one sentence for each of the calculable properties without section headers or math.

I'm curious to hear @ncclementi's thoughts on whether she thinks that's still too much information.

ncclementi commented 5 years ago

@vyasr @bjmorgan Regarding the notebooks and mybinder, I'm not sure if that would work, I run the examples on my machine Intel(R) Core(TM) i7-3820 CPU @ 3.60GHz and they took a really long time. Some of them over an hour of computation or even more. A good option would be selecting smaller problems and see how long they take on my binder. But I agree that being able to interact with the example notebooks is a good idea.

ncclementi commented 5 years ago

@vyasr @bjmorgan I think you can leave some of the numerical method section but maybe a summary and something not that technical. The reason why I suggested to remove it it is because this information is already present, almost verbatim in the Theory.ipynb notebook and example notebooks. Maybe you can point to that document instead.

Remember that the core of the paper should, according to the guidelines, contain : -A summary describing the high-level functionality and purpose of the software for a diverse, non-specialist audience -A clear statement of need that illustrates the purpose of the software

Then, any other thing you add I'd try to keep it short and for a general audience. You can go more into detail in the documentation and notebooks in the repo.

vyasr commented 5 years ago

Yes, I definitely think that MyBinder is not a good idea for very complex examples. I have not yet tried running the examples again since the first round of review. I'll do that once some more of these changes have been addressed so that you don't have too many comments at once, but I concur that putting some smaller problems for MyBinder would be nice if they can solve fast.

I'd say let's see what the proposed changes look like, then we can have a discussion about cutting further if needed. I think having some technical details is helpful to orient readers with some background, perhaps in related fields where they won't make the connection immediately without seeing a little detail. It's good for appealing to potential users who are close to the field without really being subject-matter experts.

bjmorgan commented 5 years ago

The new contributing guidelines look good. I would suggest converting the "Issue Tracker" and "pull request" text into links.

Done https://github.com/bjmorgan/pyscses/commit/1fbab4696bd4b04ea26322891f843f071ee52bba

labarba commented 5 years ago

Hi @bjmorgan, @georgiewellock — this review thread is getting a bit bloated. Would you like to create a checklist below of the items remaining for you to address, to help us keep up with things?

georgiewellock commented 5 years ago

To address the comments on the userguides:

[ ] The userguide looks pretty good overall, you should fill out or get rid of the README file there though. Also, you should look into nbsphinx, which will allow you to post the userguide as part of your RTD documentation. People shouldn't have to clone the repository to access that guide, since it's probably the more useful part of the documentation than the API. Let me know if you need examples of how this is done.

Another alternative you may want to consider is mybinder. The service isn't perfect and does go down, so I would definitely only do that in addition to RTD/nbviewer, but it's really nice to be able to execute the notebooks directly online. That's just a suggestion though, not a hard request for this paper.

To run the examples and tests I need certain notebooks that are in the repo that I can't download all together unless I clone the repo.

Is there a better way to do this? you should put a note or description on how to get the examples and tests. Since we can't run the notebooks from github directly it would be useful to add a comment on where in the repo are located instead of putting "definitions and example code can be found in the userguide." where userguide is a specific link.

we have added nbviewer links to the top level README.md as discussed:

Having said that, I do think nbviewer is also a good option if you dislike my suggestion. If you decide to stick with nbviewer, I would definitely add that to both your top-level README.md and your sphinx index.rst.

in this commit: https://github.com/bjmorgan/pyscses/commit/d58471a1a62a3e234c234bfc5ac310cd612cfbae

The userguides go through the theory of how to run the calculations, and if users want to run the calculations themselves I think it is useful to clone the whole repository.

georgiewellock commented 5 years ago

I have run through the notebooks and fixed the errors that were flagged.

[ ] Make sure the test notebooks actually run successfully; I ran into a couple of (easy-to-fix) issues such as needing to install pandas (that should just be documented at the top of the notebook), having to make the directory for storing outputs, etc.

Make sure that all your notebooks run clean and it would be useful to have information on the time it takes to run some of the examples.

I open an issue on the repo so you can give me an estimate on the timing and maybe on the future add it to the documentation, since I've been running Example 1 for about 15 min and I don't know if this is the expected behavior.

Addressed here: https://github.com/bjmorgan/pyscses/commit/b845a5d21a2748867977e5c23f38f7c6b9f168fd

georgiewellock commented 5 years ago

Going through the comments on this thread I believe we have addressed all of the software issues originally raised. We are currently redrafting the software paper which should be updated in the very near future and rewriting the summary in the README to clearly state what the problem is designed to solve.

[ ] Update release version after review
[x] Updates to software paper
[x] Updates to top level README

bjmorgan commented 5 years ago

We have revised and restructured the paper.md https://github.com/bjmorgan/pyscses/commit/6819dd1a156cc0c70b19831416f779b3a12d2aa7

Overview of changes:

We now have a short Summary section, that explains what pyscses is written to do, so that readers can check this without having to wade through the scientific background.
We have moved the scientific context to a Scientific Context section. We feel that including this is helpful for readers who might find the software useful, but who are not clear exactly how this fits with their own research. We have rewritten this content to (hopefully) make it more accessible to readers with general materials science backgrounds, but who might not be familiar with the specific physics of this problem set.
We have simplified the Numerical Model section. We feel that keeping some details about the exact model handled by pyscses is helpful for our target users (people interested in modelling ionic space-charges) who should be able to tell from scanning this paper whether pyscses is helpful for solving their models, without having to go to the repository and find the online userguide. We now give brief lists of models, calculated properties, and key approximations and limitations.

bjmorgan commented 5 years ago

We have revised the top level README: To avoid clogging up the practical instructions of how to install and then learn about using pyscses we have directed readers interested in an overview of the scientific context to the JOSS paper. https://github.com/bjmorgan/pyscses/commit/e2a315a96d05b156242cf1ecb51a6d56d06bbd21

bjmorgan commented 5 years ago

@ncclementi and @vyasr We think this addresses all the points raised so far in the review.

bjmorgan commented 5 years ago

Re: statement of need. Although the 1D Poisson-Boltzmann model is a fairly common numerical problem, we are not aware of any open source codes that solve this specifically for atomic defects in crystalline solids, or of any papers in this field that contain reproducible computational details. The updated paper contains a couple of sentences on this point.

ncclementi commented 5 years ago

@bjmorgan and @georgiewellock

Regarding the `paper.md`

I think you did a great job. My only suggestion would be to add in the summary a sentence related to the Statement of need. It is really well explained and clear by the end of the scientific context section, but I think if someone just ready the summary will miss this. Similarly I would add this to the README too, a short sentence that shows why you are different than the rest and why your software is a step forward in terms of reproducible research.

Regarding the `tests`:

I wasn't able to run the notebooks, I got an error since the folder generated_outputs doesn't exist. You should create this directory before the numpy.savetxt() line.
In the notebooks I'd add a header line that tells me what is this test about and how long it'll take to run. Similar to what you did in the examples.
I have no problems running the unit tests but it would be useful having some documentation on them that tells me what are we testing.

vyasr commented 5 years ago

I concur with the comments that @ncclementi made, those are largely identical to the issues I would have raised. One minor comment, I'd remove one of the logo files (you currently have two in your repo (one at the root and one in the resources folder). Aside from that, I think everything looks good!

bjmorgan commented 5 years ago

@vyasr These logo files are different. The logo.png in the root directory is for display in the README as a webpage (e.g. on GitHub). The pyscses logo.pdf in the resources folder is the original vector image for the logo, saved for any future use.

bjmorgan commented 5 years ago

@ncclementi

My only suggestion would be to add in the summary a sentence related to the Statement of need. It is really well explained and clear by the end of the scientific context section, but I think if someone just ready the summary will miss this. Similarly I would add this to the README too, a short sentence that shows why you are different than the rest and why your software is a step forward in terms of reproducible research.

We have added the paragraph describing the Statement of Need to the top level README. https://github.com/bjmorgan/pyscses/commit/4255c7df2a71b2b789e363799b56446c9e20fa9a

We have rearranged the paper.md so that the Statement of Need is now in the Summary, instead of the Scientific Context section. https://github.com/bjmorgan/pyscses/commit/05d2084e67668c9724d1b2465ac6c1967b8a663f

vyasr commented 5 years ago

Got it.

@labarba I'm happy with the package now. Let me know if I should copy-edit the paper, otherwise I think I'm all set on my end.

ncclementi commented 5 years ago

On my end I'd like the test notebooks to run, as I mention above there are few little problems. Regarding the paper.md it looks great now!

openjournals / joss-reviews