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 badge code:
HTML: <a href="http://joss.theoj.org/papers/1e28e12b62c0e9c8f83db62a3eff9de9"><img src="http://joss.theoj.org/papers/1e28e12b62c0e9c8f83db62a3eff9de9/status.svg"></a>
Markdown: [](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.)
@lucydot & @mkhorton, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
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 β¨
paper.md file include a list of authors with their affiliations?paper.md file include a list of authors with their affiliations?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:


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:
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:
%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/)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)]
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.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.: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.
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)
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).
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:
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.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 theverbose=Trueflag 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:
Page 2:
@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:
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
MISSING DOIs
INVALID DOIs
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:
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:
[](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:
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: