openjournals / joss-reviews

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

[REVIEW]: array_split: Multi-dimensional array partitioning #373

Closed whedon closed 6 years ago

whedon commented 7 years ago

Submitting author: @Shane-J-Latham (Shane J. Latham) Repository: https://github.com/array-split/array_split Version: 0.5.0 Editor: @arfon Reviewer: @dvalters Archive: 10.5281/zenodo.889080

Status

status

Status badge code:

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

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 questions

@dvalters, please carry out your review in this issue by updating the checklist below (please make sure you're logged in to GitHub). The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arfon know.

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 7 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS. @dvalters 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 as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS 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
dvalters commented 7 years ago

Review summary

@arfon @Shane-J-Latham

My review is quite short as I could find little to fault in the submission. The standard of the array_split software package is high. It is well documented and with a good level of testing. It provides a useful and substantial extension to the functionality of the basic NumPy array splitting functions. There are good applications for scientific parallel codes as the author suggests in the paper. The quality of the Python code is also high.

I have very few requests before the paper can be accepted, just minor issues with some of the references:

References

Could you check that DOIs are provided for papers that have them. The Dalcin (2011) paper has a DOI available on the Science Direct website, but it is not included in the .bib file. Similarly for the reference to scikit-image (https://www.ncbi.nlm.nih.gov/pubmed/25024921), and the NumPy Array reference (http://ieeexplore.ieee.org/document/5725236/).

Once these are fixed, I can recommend it be accepted into JOSS.

Suggestions (not required for acceptance)

The documentation is a good standard, but it would be really useful if at somepoint diagrams could be added to illustrate how the arrays are being split with the different functions (I find I often have to sketch out what I want the arrays/halos to look like when working on problems like this). The writing as it stands is clear, however, and this is only a suggestion for future work and not required for acceptance in my opinion.

Shane-J-Latham commented 7 years ago

Thanks for the review @dvalters, much appreciated.

Yes, I neglected to include DOIs in the 0.5.0 version, have subsequently updated them in the 0.5.x_rc branch, see this paper.bib. I'll create a 0.5.1 version, which includes the corrected paper.bib, on acceptance for the DOI.

Some diagrams in the documentation would indeed add some clarity. More generally, the ability to visualise the calculated split would also be valuable. Could plot 1D, 2D and maybe 3D splits using matplotlib, and for higher dimensions plot 2D slices at specified axis positions...

arfon commented 6 years ago

@Shane-J-Latham - please merge this PR fixing up some issues with the bibtex. Also, then please could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

array-split commented 6 years ago

Thanks for the paper.bib syntax fixes @arfon. DOI is 10.5281/zenodo.889080

arfon commented 6 years ago

@whedon set 10.5281/zenodo.889080 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.889080 is the archive.

arfon commented 6 years ago

@dvalters - many thanks for your excellent review ✨

@Shane-J-Latham - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00373 ⚡️ 🚀 💥