Joss-reviews: [REVIEW]: Basicsums: A Python package for computing structural sums and the effective conductivity of random composites

Created on 17 Mar 2019  Β·  56Comments  Β·  Source: openjournals/joss-reviews

Submitting author: @wojciech-n (Wojciech Nawalaniec)
Repository: https://bitbucket.org/basicsums/basicsums
Version: v1.0.2
Editor: @labarba
Reviewer: @lucydot, @mkhorton
Archive: 10.5281/zenodo.3253714

Status

status

Status badge code:

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

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

@lucydot & @mkhorton, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. 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 @lucydot

Conflict of interest

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.2
  • [x] Authorship: Has the submitting author (@wojciech-n) 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

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.2
  • [x] Authorship: Has the submitting author (@wojciech-n) 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?
  • [ ] 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)?
accepted published recommend-accept review

All 56 comments

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

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. 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
Attempting PDF compilation. Reticulating splines etc...

πŸ‘‹ @lucydot, @mkhorton β€” 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!

@wojciech-n I have really enjoyed reading through the documentation for the basicsums package - it is an area of materials science I have little experience in, but I certainly feel that I've learnt something (and I can see possible ways to tie this into my own future work, which is exciting). I've been working through the first two tutorials (up to Effective conductivity) and will need another few days to tick of functionality, but here are some comments so far:

Documentation : General

The tutorial clearly set out the type of problems that this package could help with, and the theory overview gives mathematical background - they form a nicely self contained explanation. Some smaller points:

  • you clearly state problems this can solve, but I not who target audience is. Some familiarity with Python is required to make use of this package, so perhaps you could add something like "A basic familiarity with Python is required" to the README.md.
  • I noticed missing references (before equation 4 in "Basic sums and the effective conductivity of composites")
  • I ran through the tutorial using a Jupyter Notebook. A link to this service, alongside a reminder to use %matplotlib inline to display plots may be useful to some users (you could also consider hosting examples as Jupyter Notebooks via the binder service, for ultimate accessibility: https://mybinder.org/)
  • In one of your examples you note that a list comprehension may take a while to run. You could suggest using the tqdm package to track progress (I am always more patient if I know something is happening :) ....), eg:
from tqdm import tqdm
res_hex_try = [BasicSums(A, cell_hex, eisenstein_indexes=[4]).esum(4)
           for A in tqdm(A_hex)]

Documentation : Testing

  • doctest has been used for (manual) testing, but there is no documentation related to this in the basicsums package. I suggest you outline the steps needed for a user to run the tests locally on the README.md.
  • It would be useful to have some indication of what proportion of the package is tested, I know this is possible with pytest and a service like codecov, I'm not sure if there is an easy way to get this information with doctest. This wouldn't block my acceptance, just a suggestion.

Documentation : Community Guidelines and citations

  • I can't see any guidelines for those who want to contribute to the software, seek support or report issues.
  • Is there a preferred way to cite your work, for people who make use of this software?

:wave: @mkhorton - how are you getting along with your review?

Hi @arfon, I haven't started this review yet, I intend to get to it next week at the latest however (when I was invited to review I communicated that I would not have time to begin before early April)

@wojciech-n I'll follow up on the review started a couple of weeks ago - let me know if you have any questions about my comments but please note I will be away on holiday with limited access to internet from 11/04-21/04 <--- This is also useful info for the editors @labarba @arfon.

Documentation

  • In the conclusion section of your paper you nicely summarise who the target audience is. JOSS review criteria states this should also be in your documentation (it might sit nicely in the introduction section?).

  • It is easy to go from the source code to the documentation, but not vice verca - perhaps you could provide a link in the documentation (https://basicsums.bitbucket.io/) to the source (https://bitbucket.org/basicsums/basicsums)

Software Paper

  • I suggest you re-word the module descriptions in the software paper as I do not think these are accessible to a general audience; I found the language quite technical - particularly the descriptions for the weierstrass and eisenstein modules. Perhaps the figure and a short description of the key components would suffice rather than a description for each module.

  • You summarise the range of research which uses structural sums. Please state which of these projects have used this piece of software to do that work (if any).

Functionality

  • I've worked through the first three tutorials so I am happy to tick this off. Providing functions for loading the data eg: load_example01() makes the tutorials nice and neat, but it's not obvious what form the A and r parameters should take in show_disks(). A short description eg. where A is a NumPy array of complex numbers specifying the disk centres and r is a NumPy array of floats corresponding to the disk radii would be good for those who want to quickly try out their own tests without having to refer to API documentation.

@whedon assign @labarba as editor

@wojciech-n β€” Do keep us updated when you work through the set of review comments from @lucydot.

@mkhorton β€” Is your schedule clearing to start working on your review if this submission? Let us know your status. Thanks!

Yes, I’ve started now. Will update shortly!

On Wed, Apr 10, 2019 at 09:41, Lorena A. Barba notifications@github.com
wrote:

@mkhorton https://github.com/mkhorton β€” Is your schedule clearing to
start working on your review if this submission? Let us know your status.
Thanks!

β€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openjournals/joss-reviews/issues/1327#issuecomment-481768399,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC1rRP8MF2Kq15kABI1Sp30lq5af72sXks5vfhQqgaJpZM4b4GPY
.

@wojciech-n I'm also having the same issues @lucydot had with the testing. It wasn't clear to me how the tests are meant to be run at first. This definitely should be included in the documentation. I also wasn't able to find any tests for anything other than Weierstrass? The other key classes definitely should have at least one test also.

@lucydot doctest.testfile("weierstrass.doctest") did work for me to run the tests, which successfully reported TestResults(failed=0, attempted=27) (note this number is the number of lines evaluated, the number of actual tests which is 4). I don't have a copy of Mathematica to hand to verify the .dat files however so I'm going to have those numbers on on faith.

I do like the choice of using doctest and integrating tests into documentation, it makes the tests very readable and makes it clear what the intent of the code is.

@lucydot, @mkhorton thank you very much for your valuable notes. @labarba thank you for editing my submission. I am going to revise both the docs and the package and let you know. Unfortunately I have been very busy lately, hence please let me know what is the deadline.

Well, if you could give this a couple of weeks, that would be nice. Take it as a soft deadline.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

Dear Editor @labarba, please excuse me such a delay, I just wanted to prepare the revision properly.

Dear Reviewers, @lucydot and @mkhorton, thank you for your time spent for reviewing my software. I am so glad that there were things you liked about the package. I revised documentation and the package as v1.0.2. I tried to take into account all of your notes. Hope I did not miss anything and this will meet your expectations. Please, let me answer them below.

Reviewer @lucydot

Documentation : General
you clearly state problems this can solve, but I not who target audience is. Some familiarity with Python is required to make use of this package, so perhaps you could add something like "A basic familiarity with Python is required" to the README.md.

I added the subsection "Target audience" to the introduction section (https://basicsums.bitbucket.io/intro.html#target-audience). The README.rst in the repository is updated as adviced (in the requirements section).

I noticed missing references (before equation 4 in "Basic sums and the effective conductivity of composites")

Thank you for pointing this out. I fixed this.

I ran through the tutorial using a Jupyter Notebook. A link to this service, alongside a reminder to use %matplotlib inline to display plots may be useful to some users (you could also consider hosting examples as Jupyter Notebooks via the binder service, for ultimate accessibility: https://mybinder.org/)

I placed both the link and the backend hint to the beginning of the tutorial section (https://basicsums.bitbucket.io/tutorial0.html). I was not aware of the binder service. I am going to use it in near future for tutorial's files.

In one of your examples you note that a list comprehension may take a while to run. You could suggest using the tqdm package to track progress (I am always more patient if I know something is happening :) ....), eg:

Great advice, thanks! I noted this option as a hint in the tutorial (near to the and of Example 1 https://basicsums.bitbucket.io/tutorial_geometric_parameters.html#example-1-visualization-of-dynamic-changes-of-the-system).

Documentation : Testing

doctest has been used for (manual) testing, but there is no documentation related to this in the basicsums package. I suggest you outline the steps needed for a user to run the tests locally on the README.md.

I added the functionality of running tests after the package installation. I mentioned it in the documentation as well as in the README.rst file in the repository. Tests are still based solely on doctests: the script runs all doctests from doctrings as well as from additional testfiles residing in the tests submodule.

It would be useful to have some indication of what proportion of the package is tested, I know this is possible with pytest and a service like codecov, I'm not sure if there is an easy way to get this information with doctest. This wouldn't block my acceptance, just a suggestion.

Thank you for your suggestion about the codecov, I would consider this in the future. Just need some time fo this.

Documentation : Community Guidelines and citations

I can't see any guidelines for those who want to contribute to the software, seek support or report issues.
Is there a preferred way to cite your work, for people who make use of this software?

I placed Contribution and citations subsection in the Introduction (https://basicsums.bitbucket.io/intro.html#contribution-and-citations).

Reviewer @mkhorton

Documentation

In the conclusion section of your paper you nicely summarise who the target audience is. JOSS review criteria states this should also be in your documentation (it might sit nicely in the introduction section?).

I added the subsection "Target audience" to the introduction section (https://basicsums.bitbucket.io/intro.html#target-audience).

It is easy to go from the source code to the documentation, but not vice verca - perhaps you could provide a link in the documentation (https://basicsums.bitbucket.io/) to the source (https://bitbucket.org/basicsums/basicsums)

The link is now available at https://basicsums.bitbucket.io/ (just before the Contents).

Software Paper

I suggest you re-word the module descriptions in the software paper as I do not think these are accessible to a general audience; I found the language quite technical - particularly the descriptions for the weierstrass and eisenstein modules. Perhaps the figure and a short description of the key components would suffice rather than a description for each module.

Very good suggestion indeed. I shortened the description as advised.

You summarise the range of research which uses structural sums. Please state which of these projects have used this piece of software to do that work (if any).

This information is now present in the paper.

Functionality

I've worked through the first three tutorials so I am happy to tick this off. Providing functions for loading the data eg: load_example01() makes the tutorials nice and neat, but it's not obvious what form the A and r parameters should take in show_disks(). A short description eg. where A is a NumPy array of complex numbers specifying the disk centres and r is a NumPy array of floats corresponding to the disk radii would be good for those who want to quickly try out their own tests without having to refer to API documentation.

I added the note in place where load_example01() is used.

@wojciech-n I'm also having the same issues @lucydot had with the testing. It wasn't clear to me how the tests are meant to be run at first. This definitely should be included in the documentation. I also wasn't able to find any tests for anything other than Weierstrass? The other key classes definitely should have at least one test also.

I added the functionality of running tests after the package installation. I mentioned it in the documentation as well as in the README.rst file in the repository. Tests are still based solely on doctests: the script runs all doctests from doctrings as well as from additional testfiles residing in the tests submodule.

Hello @wojciech-n,
Thanks for your clear response to the reviews. I've ticked off all the boxes and am happy to recommend for acceptance but have two minor comments:

  • The post-installation testing worked well for me and it is nice and straight forward to use. In the documentation I would mention that the second command from basicsums.tests import test... is done from the python prompt (switching across from bash used prior), and that the verbose=True flag can be used for more detailed output.
    -I couldn't see the information about testing in the README.rst.

@mkhorton β€” Would you have another look at this submission? The author has done various improvements.

@labarba ok, will do!

On Tue, Jun 4, 2019 at 13:26, Lorena A. Barba notifications@github.com
wrote:

@mkhorton https://github.com/mkhorton β€” Would you have another look at
this submission? The author has done various improvements.

β€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openjournals/joss-reviews/issues/1327?email_source=notifications&email_token=AAWWWRAVDW6EB6ZE4S2MMTTPY3FVNA5CNFSM4G7AMPMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW5YTXI#issuecomment-498829789,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWWWRCVXYSRTBODOA6XTALPY3FVNANCNFSM4G7AMPMA
.

@mkhorton - any news on this review?

Hi @danielskatz, the tests have been expanded to cover additional classes and I can verify they all pass successfully. The other issues seemed to be addressed. The only remaining item is community guidelines, otherwise it seems to be good to go.

Dear Reviewers, thank you for checking the software and package again.

@lucydot,

I would mention that the second command from basicsums.tests import test... is done from the python prompt (switching across from bash used prior), and that the verbose=True flag can be used for more detailed output.

Of course, I should have mentioned this. Documentation is updated now: https://basicsums.bitbucket.io/intro.html#installation

I couldn't see the information about testing in the README.rst.

I placed the information in the Installation section (https://bitbucket.org/basicsums/basicsums/src/master/README.rst).

@mkhorton,

The only remaining item is community guidelines, otherwise it seems to be good to go.

I placed Contribution and citations subsection in the Introduction (https://basicsums.bitbucket.io/intro.html#contribution-and-citations). Should I extend this description?

@lucydot β€” I see that all your checklist items are ticked off. Do we have your recommendation to accept?

@mkhorton β€” Your last item is Community Guidelines. Are you satisfied with that?

@labarba yes, it's recommendation to accept from me..

Yes, fine from me. Apologies I’d missed that.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@wojciech-n β€” You will need to fix the citation syntax in several places. When the citation is part of the sentence, then use: Author (year). If the citation is not part of the sentence, then it should appear in parenthesis. E.g. β€œWe take inspiration from Smith et al. (2006) to … β€œ / β€œThe code implements Theory 1 (Jones 2012) and Theory 2 (Roberts 2014) to obtain … β€œ For the syntax to obtain brackets in the right places, see: https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html#citation_syntax

These sentences need fixing for in-text citations:

  • For example of obtaining symbolic-numerical formula for the EC of random materials using a Monte Carlo method, see (Czapla, Mityushev, & Nawalaniec, 2012). For more theory and applications, see (Gluzman et al., 2018).
  • For example, (Mityushev & Nawalaniec, 2015) used structural sums in the systematic investigation of dynamically changing microstructures.
  • For the precise de nition of structural sums, the EC, and the underlying geometry, see (Gluzman et al., 2018)
  • The basicsums module has contributed to a number of research results, e.g. (Nawalaniec, 2019), (Nawalaniec, 2017).
  • It was also used in several projects of the Materialica+ Research Group, presented in (Gluzman et al., 2018).
  • (page 2) For more details, see (Nawalaniec, 2019) and documentation.
  • avoid the double parenthesis in (Nawalaniec, 2016), (Nawalaniec, 2017) by using a semicolon to separate the two citations
  • the paper (Nawalaniec, 2017) describes

Page 2:

  • "being convolutions of elliptic functions functions." >> delete repeated word
  • "in details algorithms and methods" >> in detail
  • "helpful for scientists from the eld of computational materials science" >> missing period at the end

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@labarba, thank you for your detailed notes on references. I corrected them, as well as typos from page 2.

@whedon accept

No archive DOI set. Exiting...

Oops. We need to do the final steps before acceptance.

Please do the following:

  • [x] Please make a tagged release of your software, and list the version tag of the archived version here.
  • [x] Archive the reviewed software in Zenodo
  • [x] Check the Zenodo deposit has the correct metadata, this includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it); you may also add the authors' ORCID.
  • [x] Please list the Zenodo DOI of the archived version here.

You may find this helpful: https://guides.github.com/activities/citable-code/

@labarba, I hope I did everything correctly:

Apologies for the lag, @wojciech-n !
I'm not familiar with how Bitbucket organizes things, but I poked around and could not find a tagged version. I landed here:
https://bitbucket.org/basicsums/basicsums/downloads/?tab=tags
Help me here?

@whedon set 10.5281/zenodo.3253714 as archive

OK. 10.5281/zenodo.3253714 is the archive.

@labarba,

I'm not familiar with how Bitbucket organizes things, but I poked around and could not find a tagged version.

Excuse me, this is my fault: I did not push the tag to Bitbucket. Now it seems to be OK: https://bitbucket.org/basicsums/basicsums/downloads/?tab=tags

@whedon set v1.0.2 as version

OK. v1.0.2 is the version.

@whedon accept

Attempting dry run of processing paper acceptance...

```Reference check summary:

OK DOIs

  • 10.1016/j.commatsci.2014.09.020 is OK
  • 10.1016/j.commatsci.2012.05.058 is OK
  • 10.3934/mbe.2017018 is OK
  • 10.1007/978-3-319-12577-0_84 is OK
  • 10.1002/9783527621408.ch5 is OK
  • 10.1016/j.compstruct.2017.09.048 is OK
  • 10.1007/s40314-017-0449-6 is OK
  • 10.1016/j.jsc.2015.08.001 is OK
  • 10.1016/C2016-0-00654-X is OK
  • 10.1098/rspa.2018.0698 is OK
  • 10.1090/mmono/079 is OK
  • 10.1007/978-3-642-66209-6 is OK
  • 10.1002/9783527621408.ch5 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/824

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/824, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.
@whedon accept deposit=true

@whedon accept deposit=true

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

🐦🐦🐦 πŸ‘‰ Tweet for this paper πŸ‘ˆ 🐦🐦🐦

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/825
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01327
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! πŸŽ‰πŸŒˆπŸ¦„πŸ’ƒπŸ‘»πŸ€˜

    Any issues? notify your editorial technical team...

Congratulations, @wojciech-n, your JOSS paper is published! πŸš€

Huge thanks to the reviewers: @lucydot, @mkhorton β€” your contributions to JOSS are what keeps this adventure going! πŸ™

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01327/status.svg)](https://doi.org/10.21105/joss.01327)

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

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

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Was this page helpful?
0 / 5 - 0 ratings