search

openjournals / joss-reviews

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

[REVIEW]: PyStokes: Stokesian hydrodynamics in Python #2318

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @rajeshrinet (Rajesh Singh) Repository: https://github.com/rajeshrinet/pystokes Version: v2.2.0 Editor: @harpolea Reviewer: @fcooper8472, @khinsen Archive: 10.5281/zenodo.3923867

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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) by leaving comments 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

@fcooper8472 & @khinsen, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @harpolea know.

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

Review checklist for @fcooper8472

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @khinsen

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @fcooper8472, @khinsen it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1137/1.9780898718003.bm is OK
- 10.1007/978-3-319-12787-3 is OK
- 10.1007/BF01194638 is OK
- 10.1063/1.5090179 is OK
- 10.1088/2399-6528/aaab0d is OK
- 10.1103/PhysRevLett.117.228002 is OK
- 10.1039/C7CS00461C is OK
- 10.1039/B918598D is OK
- 10.1146/annurev.fl.21.010189.000425 is OK
- 10.1103/PhysRevLett.124.088003 is OK
- 10.1017/S0022112075001486 is OK
- 10.1017/S0022112082000627 is OK
- 10.1017/CBO9780511624124 is OK
- 10.1017/S0022112095003260 is OK
- 10.1016/j.enganabound.2004.12.001 is OK
- 10.1073/pnas.1718807115 is OK

MISSING DOIs

- https://doi.org/10.2307/3613759 may be missing for title: The mathematical theory of viscous incompressible flow
- https://doi.org/10.1080/17797179.2017.1294829 may be missing for title: Fluctuating hydrodynamics and the Brownian motion of an active colloid near a wall
- https://doi.org/10.1088/1742-5468/2015/06/p06017 may be missing for title: Many-body microhydrodynamics of colloidal particles with active boundary layers

INVALID DOIs

- None
whedon commented 4 years ago

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

khinsen commented 4 years ago

@harpolea I did a first pass looking at the code and the paper. It was a lot of fun: this is a very nice implementation of hydrodynamic and phoretic interactions which invites users to play with it, in particular by including visualization tools, and that is just great for doing research. The next time I do work in this domain, I'll happily abandon my own 25-year-old Fortran library and use PyStokes instead!

I have made a few suggestions for improvements via issues/pull requests on the code repository, which the authors have handled immediately. Interactive reviewing is a fun experience :-)

There are a few open points in the review checklist as of today:

  1. There are no tests, only examples. This being number-crunching code, it is very difficult to write tests that are meaningful and predictable, given how little control programmers have over the details of float arithmetic (shameless self-advertising: The Approximation Tower in Computational Science: Why Testing Scientific Software Is Difficult). Does JOSS have an editorial policy concerning tests for number-crunching software? Or at least recommendations for authors? I didn't find anything.

  2. Community guidelines: There are instructions for reporting issues and for seeking support, but not for contributing to the software. See https://github.com/rajeshrinet/pystokes/issues/6.

  3. High-level summary in the paper: The current summary is clear enough for potential users of the software, but maybe not "for a diverse, non-specialist audience". But this should best be judged by someone unfamiliar with the field.

  4. State of the field: the paper doesn't provide comparisons with other commonly-used packages, but then there are no competing commonly used packages, as far as I can tell. But the authors should definitely say something about this, even if it's only a confirmation of my suspicion.

khinsen commented 4 years ago

Contribution guidelines were just added!

harpolea commented 4 years ago

@khinsen Thank you for your review!

As you probably read, the JOSS reviewer criteria for testing can be found here. While not required, it is strongly encouraged that some kind of automated testing suite is set up to test the functionality of the code.

As you said, testing codes such as these can be tricky. I am not that familiar with this particular branch of CFD, however I imagine there may be some simple, standard test problems with (semi-)analytic solutions? E.g. simple advection tests, tests to demonstrate energy conservation, etc

Regression tests could also be another option. In the codes I work on, we save the output from a selection of problems as benchmarks, then test against these whenever changes are made to the code to ensure that they do not change the output.

rajeshrinet commented 4 years ago

@khinsen thanks very much for your review and for several suggestions on the project's page. Thanks @harpolea for suggestions on tests. I would like to note that there are some tests, which run every time someone pushes to the repo (all the example notebooks are also tested). The folder containing tests was somewhat hidden, pystokes/tests. I have now moved it to the main folder. The current testing is done to ensure that the translational and rotational speed of a particle - in an unbounded domain and near a plane wall - is consistent with solutions known in the literature.

rajeshrinet commented 4 years ago

I have now added one more test: compare mobility of a sedimenting lattice with known results in literature (Zick and Homsy, 1982) and moved tests at start of folder tree.

khinsen commented 4 years ago

@rajeshrinet I had indeed overlooked the tests! Thanks for making them more visible. And yes, the old Zick/Homsy paper makes for a nice test based on well-known results. Could you add instructions to the README for how to run the tests? That would also ensure their visibility.

rajeshrinet commented 4 years ago

@khinsen I have now added it to the README. Thanks for this suggestion.

khinsen commented 4 years ago

@rajeshrinet @harpolea Looks good (and the tests all pass on my machine)! I am happy with the software in its current state. I have suggestions for a revision of the paper (see above), but you may prefer to wait for @fcooper8472's feedback before attacking the paper.

fcooper8472 commented 4 years ago

I think the software looks very good.

I found a few problems with installation, but @rajeshrinet addressed those very quickly - it's great for this to be happening in real time!

In terms of the formal checklist, I have the same two checkboxes un-checked as @khinsen:

Other than that, a couple of suggestions:

rajeshrinet commented 4 years ago

@fcooper8472 and @khinsen many thanks for your comments above and for several suggestions on the repo which have already helped improved the codebase. I am now working on the paper and would update it in the light of your comments. I will post a comment here when done.

@fcooper8472, indeed PyStokes was first written in Python 2 and was later modified to support Python 3. Thus, it supports both Python 2 and 3. I have now added GitHub workflow which checks both Python 2 and 3. See the result at: https://github.com/rajeshrinet/pystokes/runs/806514423. I now mention this in the README as well. I am now working on getting the coverage bit up and running.

fcooper8472 commented 4 years ago

@rajeshrinet this is great!

Just to be clear I do not think the general suggestions I have made about PyStokes are strictly required for publication in JOSS, though it is great that you're addressing these.

rajeshrinet commented 4 years ago

@harpolea: thank you very much for your editorial assistance in reviewing our submission. We, the authors (@rajeshrinet and @ronojoy), have revised the manuscript in response to the reviewers suggestions. We detail the changes below and hope that, with these changes, the manuscript can now accepted in JOSS.

Referee: @khinsen.

@harpolea I did a first pass looking at the code and the paper. It was a lot of fun: this is a very nice implementation of hydrodynamic and phoretic interactions which invites users to play with it, in particular by including visualization tools, and that is just great for doing research. The next time I do work in this domain, I'll happily abandon my own 25-year-old Fortran library and use PyStokes instead!

I have made a few suggestions for improvements via issues/pull requests on the code repository, which the authors have handled immediately. Interactive reviewing is a fun experience :-)

We thank the referee for a positive evaluation of our code and paper. It has indeed been fun to interactively improve the codebase through discussion on the issue tracker. Responses to specific comments are below:

There are a few open points in the review checklist as of today:

  1. There are no tests, only examples. This being number-crunching code, it is very difficult to write tests that are meaningful and predictable, given how little control programmers have over the details of float arithmetic (shameless self-advertising: The Approximation Tower in Computational Science: Why Testing Scientific Software Is Difficult). Does JOSS have an editorial policy concerning tests for number-crunching software? Or at least recommendations for authors? I didn't find anything.

We have now upgraded our testing and made it more visible. We followed the referee's suggestion to add details to the REAMDE on how to test the code.

  1. Community guidelines: There are instructions for reporting issues and for seeking support, but not for contributing to the software. See rajeshrinet/pystokes#6.

A CONTRIBUTING.md was added.

  1. High-level summary in the paper: The current summary is clear enough for potential users of the software, but maybe not "for a diverse, non-specialist audience". But this should best be judged by someone unfamiliar with the field.

We have now added a new paragraph at the start of the paper and added a new figure. Both are at page 1. We hope that this will be helpful as a high-level summary.

  1. State of the field: the paper doesn't provide comparisons with other commonly-used packages, but then there are no competing commonly used packages, as far as I can tell. But the authors should definitely say something about this, even if it's only a confirmation of my suspicion.

We have now clearly described on page 3 of our paper that hydrodynamic interactions of active particles are obtained in terms of mobility matrices and propulsion tensors. Two libraries which compute mobility matrices have been cited, while we are not aware of any library, other than PyStokes, that evaluates propulsion tensors to compute hydrodynamic interactions of active particles.

Referee: @fcooper8472

I think the software looks very good.

I found a few problems with installation, but @rajeshrinet addressed those very quickly - it's great for this to be happening in real time!

In terms of the formal checklist, I have the same two checkboxes un-checked as @khinsen:

We thank the referee for a positive evaluation and comments on our submission. It was indeed very helpful in improving installation instructions using PyPI.

  • Summary: I have some experience in this area (I once coded up a spectral method for solving Navier-Stokes on a periodic domain), but I am not a domain expert and do not understand the whole of the first paragraph. Even after a bit of time on Google I'm still not certain I know what the phoretic field is, for instance. I think a "diverse non-specialist audience" would benefit from a paragraph up-front with a higher level overview of what the software is designed to do.
  • State of the field: I cannot tell from reading the paper whether there is other software available that does a similar task. Either way, I think this needs stating in the paper.

As mentioned in response to referee A (@khinsen), we have now revised the paper and added more details for a non-specialist audience including a figure on page 1 which gives a high-level summary.

Other than that, a couple of suggestions:

  • The README (until very recently) stated that PyStokes works with Python 2.6+ or Python 3.4+: I've not been able to verify this, but it would be very useful for users to know which versions it works with. It would be very easy to add a GitHub workflow to test on multiple Python version. @rajeshrinet would you be interested in a PR that implements that?

We have now updated the testing and it is available for both python 2.7 and 3.7 using GitHub workflow. We have also added a badge in the README: Python Version.

  • Testing (outside of checking that examples run - which is great!) is fairly thin. I would encourage coverage checking (suggested here to assess what proportion of code is covered by tests, but on top of that it would be great to see a unit testing framework in use. I understand that some of the number crunching can be very hard to test effectively: but even boring things like the utils would benefit from some testing.

As discussed here, coverage checking for a cython codebase leads to performance loss. Thus, we have decided against coverage checking, pending a solution that does not compromise on speed.

khinsen commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

harpolea commented 4 years ago

@rajeshrinet Great – thanks for your quick response!

@khinsen and @fcooper8472: are you happy with these changes, and do you think that they satisfy the requirements to check off the last points in the review checklist?

fcooper8472 commented 4 years ago

The additional paragraph at the start really helps - it's clear and certainly makes it more accessible to a non-specialist.

The only remaining concern is still about other software packages. I still do not think there is an assertion that no other similar software exists, or a description of other packages, so I'm still not 100% sure I can tick

"Do the authors describe how this software compares to other commonly-used packages?"

@rajeshrinet are there other packages that do anything in the same vein? If not could you add a sentence to this effect?

rajeshrinet commented 4 years ago

@fcooper8472 thanks for your comments.

A search on GitHub does not find anything comparable to PyStokes. There are libraries, such as Ludwig (from EPCC, Edinburgh) which simulates the hydrodynamics of point particles, resolving the fluid degrees of freedom. It does not include phoretic interactions. In terms of the functionality of simultaneously resolving both phoretic and hydrodynamic interactions, we think that PyStokes is, as of this writing, unique. Thus we note in our revision that:

To the best of our knowledge, PyStokes is the only numerical implementation of propulsion tensors to model suspensions of active particles.

fcooper8472 commented 4 years ago

Ah! My apologies @rajeshrinet, I missed that sentence.

@harpolea I can confirm that the current version is acceptable as far as I'm concerned.

harpolea commented 4 years ago

Great, thanks @fcooper8472 !

khinsen commented 4 years ago

@harpolea @rajeshrinet I am happy with the updated paper as well. Figure 1 in particular is a very nice summary of how everything fits together. The authors' description of the state of the art looks accurate as well. So... I just ticked off the last boxes of my checklist! Ready for take-off!

khinsen commented 4 years ago

Ahhh.... a minor nitpick I noticed while closing the open PDF file! The proper title for Odqvist 1930 is "Über die Randwertaufgaben der Hydrodynamik zäher Flüssigkeiten", with all capitalization required by German grammar.

rajeshrinet commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

rajeshrinet commented 4 years ago

@fcooper8472 and @khinsen thanks very much for your comments. @khinsen thanks for suggesting fix in the reference. It has now been updated.

harpolea commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1137/1.9780898718003.bm is OK
- 10.1137/1.9781611973242 is OK
- 10.1007/978-3-319-12787-3 is OK
- 10.1007/BF01194638 is OK
- 10.1063/1.5090179 is OK
- 10.1088/2399-6528/aaab0d is OK
- 10.1103/PhysRevLett.117.228002 is OK
- 10.1039/B918598D is OK
- 10.1146/annurev.fl.21.010189.000425 is OK
- 10.1103/PhysRevLett.124.088003 is OK
- 10.1017/S0022112075001486 is OK
- 10.1017/S0022112082000627 is OK
- 10.1017/CBO9780511624124 is OK
- 10.1017/S0022112095003260 is OK
- 10.1016/j.enganabound.2004.12.001 is OK
- 10.1073/pnas.1718807115 is OK
- 10.1016/0010-4655(95)00029-F is OK
- 10.1146/annurev.fl.09.010177.002011 is OK

MISSING DOIs

- https://doi.org/10.2307/3613759 may be missing for title: The mathematical theory of viscous incompressible flow
- https://doi.org/10.1080/17797179.2017.1294829 may be missing for title: Fluctuating hydrodynamics and the Brownian motion of an active colloid near a wall
- https://doi.org/10.1088/1742-5468/2015/06/p06017 may be missing for title: Many-body microhydrodynamics of colloidal particles with active boundary layers

INVALID DOIs

- None
harpolea commented 4 years ago

Thanks @khinsen!

@rajeshrinet: I'm just going through the references now. As whedon pointed out, there are a couple that are missing DOIs (the first one doesn't seem to match up, but the latter two look good to me). Would you be able to add them?

rajeshrinet commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1137/1.9780898718003.bm is OK
- 10.1137/1.9781611973242 is OK
- 10.1007/978-3-319-12787-3 is OK
- 10.1007/BF01194638 is OK
- 10.1063/1.5090179 is OK
- 10.1088/2399-6528/aaab0d is OK
- 10.1080/17797179.2017.1294829 is OK
- 10.1103/PhysRevLett.117.228002 is OK
- 10.1088/1742-5468/2015/06/p06017 is OK
- 10.1039/B918598D is OK
- 10.1146/annurev.fl.21.010189.000425 is OK
- 10.1103/PhysRevLett.124.088003 is OK
- 10.1017/S0022112075001486 is OK
- 10.1017/S0022112082000627 is OK
- 10.1017/CBO9780511624124 is OK
- 10.1017/S0022112095003260 is OK
- 10.1016/j.enganabound.2004.12.001 is OK
- 10.1073/pnas.1718807115 is OK
- 10.1016/0010-4655(95)00029-F is OK
- 10.1146/annurev.fl.09.010177.002011 is OK

MISSING DOIs

- https://doi.org/10.2307/3613759 may be missing for title: The mathematical theory of viscous incompressible flow

INVALID DOIs

- None
harpolea commented 4 years ago

@rajeshrinet thanks! I've made a PR with fixes to a couple of the references. One was missing a title, and for another the DOI pointed to a different article

harpolea commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

rajeshrinet commented 4 years ago

@rajeshrinet thanks! I've made a PR with fixes to a couple of the references. One was missing a title, and for another the DOI pointed to a different article

Thanks @harpolea. It has been merged.

harpolea commented 4 years ago

@rajeshrinet Thanks!

At this point could you:

I can then move forward with accepting the submission.

rajeshrinet commented 4 years ago

@harpolea ,

harpolea commented 4 years ago

@rajeshrinet the Zenodo release seems to have been mistakenly dated to November 2014. Could you fix that? (I think you should be able to edit the metadata?)

harpolea commented 4 years ago

@rajeshrinet I see that's now fixed - must have just been a Zenodo bug!

rajeshrinet commented 4 years ago

@harpolea thanks, just fixed it:

Screenshot 2020-06-30 at 1 57 23 PM

harpolea commented 4 years ago

@whedon set 10.5281/zenodo.3923867 as archive

whedon commented 4 years ago

OK. 10.5281/zenodo.3923867 is the archive.

harpolea commented 4 years ago

@whedon set v2.2.0 as version

whedon commented 4 years ago

OK. v2.2.0 is the version.

harpolea commented 4 years ago

@rajeshrinet: this all looks good to me, so I'm going to go ahead and recommend this for acceptance

@khinsen and @fcooper8472: thank you again for your work reviewing this submission!

harpolea commented 4 years ago

@whedon accept

whedon commented 4 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1137/1.9780898718003.bm is OK
- 10.1007/978-3-319-12787-3 is OK
- 10.1007/BF01194638 is OK
- 10.1063/1.5090179 is OK
- 10.1088/2399-6528/aaab0d is OK
- 10.1080/17797179.2017.1294829 is OK
- 10.1103/PhysRevLett.117.228002 is OK
- 10.1088/1742-5468/2015/06/p06017 is OK
- 10.1039/B918598D is OK
- 10.1146/annurev.fl.21.010189.000425 is OK
- 10.1103/PhysRevLett.124.088003 is OK
- 10.1017/S0022112075001486 is OK
- 10.1017/S0022112082000627 is OK
- 10.1017/CBO9780511624124 is OK
- 10.1017/S0022112095003260 is OK
- 10.1016/j.enganabound.2004.12.001 is OK
- 10.1073/pnas.1718807115 is OK
- 10.1016/0010-4655(95)00029-F is OK
- 10.1146/annurev.fl.09.010177.002011 is OK

MISSING DOIs

- https://doi.org/10.2307/3613759 may be missing for title: The mathematical theory of viscous incompressible flow

INVALID DOIs

- None
whedon commented 4 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

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

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

@whedon accept deposit=true