search

openjournals / joss-reviews

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

[REVIEW]: Rescal-snow: A model for dunes and snow-waves #1699

Closed whedon closed 4 years ago

whedon commented 5 years ago

Submitting author: @kellykochanski (Kelly Kochanski) Repository: https://github.com/kellykochanski/rescal-snow Version: v1.1.1 Editor: @kthyng Reviewers: @zbeekman, @rgmyr Archive: 10.5281/zenodo.3517217

Status

status

Status badge code:

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

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

@zbeekman & @rgmyr, 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 @kthyng know.

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

Review checklist for @zbeekman

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @rgmyr

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @zbeekman it looks like you're currently assigned to review 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

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

@whedon generate pdf
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:

kthyng commented 5 years ago

I just saw that I closed the pre-review issue before whedon had actually added the second reviewer. I have gone in and edited the first comment by hand to add @rgmyr. However, I see that he is not listed as an "Assignee" at the top right of the page, and I can't add him. @arfon is this important? Will @rgmyr have all necessary permissions the way I have added him?

arfon commented 5 years ago

@whedon add @rgmyr as reviewer

whedon commented 5 years ago

OK, @rgmyr is now a reviewer

arfon commented 5 years ago

@whedon add @rgmyr as reviewer

@kthyng doing this ☝️makes Whedon give the reviewer the permission on the repository (although it doesn't make a new checklist).

@rgmyr will need to accept the invite here: https://github.com/openjournals/joss-reviews/invitations

kthyng commented 5 years ago

Thanks @arfon! Looks like both reviewers are able to make edits now.

rgmyr commented 5 years ago

Minor issue on the paper affiliations list: the '2' for LLNL is in the wrong place (comes after instead of before). I think this is because in paper.md, that line should just be index: 2 instead of - index: 2.

rgmyr commented 5 years ago

I've had a couple of (possibly related) installation issues, which I've created tickets for:

In the meantime, I now at least have the software running and will try to continue with the rest of the Functionality and Documentation points.

EDIT: I should note that it's possible these issues are/were addressed in docs/how_to_install.md, which is linked to in the Installation section of the README, but that file seems to no longer exist.

kellykochanski commented 5 years ago

@whedon generate pdf

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:

kellykochanski commented 4 years ago

@rgmyr We've updated the build system and installation instructions to fix these issues. Zlib is now a dependency and is not explicitly linked within Rescal-snow.

kellykochanski commented 4 years ago

@zbeekman Quick reminder - we're looking forward to your review.

rgmyr commented 4 years ago

My installation issues were fixed by the build update and additional dependency instructions.

My only other change request was regarding documentation and style consistency of the included Python utilities, which @kellykochanski promptly improved:

I should note that I wasn't able to test the functionality related to parallel simulation runs, simply because I don't have access to any remote computing clusters at this time. With that said, it looks pretty straightforward and reasonable based on my previous experiences with slurm-scheduled systems.

That completes my review checklist, so I'm happy to recommend acceptance of this submission.

zbeekman commented 4 years ago

@kellykochanski I just wanted to update you on my progress. (I know you're busy responding to issues & coding so no need to respond.) So far I have:

To conclude the review I will do the following (out of time today):

Outstanding issues (so far) that I think need to be resolved, in order of severity:

Some CI testing and/or single test script to verify the tutorial works would have probably prevented https://github.com/kellykochanski/rescal-snow/issues/41 and would be really good to add. (You probably cannot do the last parametric parameter study example on a free CI solution, but a script for including this could be a good supplement to CI testing here.

So far things look really good! I think https://github.com/kellykochanski/rescal-snow/issues/41 definitely should be addressed/resolved before publication, but most of the other things are pretty minor.

kellykochanski commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

kellykochanski commented 4 years ago

@zbeekman I believe these issues have now been addressed; issues 35 and 37 are closed, and it would be great if you could confirm our work on 41.

I'm interested inseeing your results for the performance test.

kellykochanski commented 4 years ago

@zbeekman Do you know when you will have time to look at this?

zbeekman commented 4 years ago

On the agenda for today! Thanks for the nudge.

On Mon, Oct 14, 2019 at 10:56 AM Kelly Kochanski notifications@github.com wrote:

@zbeekman https://github.com/zbeekman Do you know when you will have time to look at this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1699?email_source=notifications&email_token=AACEIPBB6G35WTGNYX2WRYLQOSCB5A5CNFSM4IS7QOVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBFB6PQ#issuecomment-541728574, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACEIPFN3CXMAKJ7NG6T6L3QOSCB5ANCNFSM4IS7QOVA .

zbeekman commented 4 years ago

I think I have more or less concluded my review. There are many great features of rescal-snow, and, in most circumstances, building it was straight forwards. I've tested it on:

In general there is a fairly large quantity of usage documentation and tutorials. At times this has been a double edged sword because the documentation and tutorials are markdown documents which can become out of sync with the project and software. Overall, the software testing and processes relating to reproducibility and quality assurance do leave some room for improvement. Despite this, these areas have improved over the course of this review, which is encouraging.

I am recommending publication with minor revisions, discussed in https://github.com/kellykochanski/rescal-snow/issues/41 and https://github.com/kellykochanski/rescal-snow/issues/44. The main software package seems fairly robust and has some really interesting capabilities. Some of the supporting scripts and pre/post-processing tools do not appear to be as robust as I would like, and caused me to devote more time than I would have liked to troubleshooting. Nevertheless, I feel that the overall package meets the requirements for JOSS publication. The weakest points, in my opinion, are in the Documentation section, in particular, Automated Tests and Functionality Documentation. The Functionality Documentation issues were because following the examples did not produce the results they claimed to due to bugs, most of which are fixed.

While I do not believe the aforementioned issues preclude rescal-snow from being published--i.e., "major revisions",--I recommend that the authors take additional steps (now or after publication) to reduce the brittleness and manual nature of a lot of the testing and documentation:

  1. Setup CI testing
    • test on linux
    • test on macOS
    • test scripts (Bash and Python) used in tutorials and user-documentation
    • setup unit tests for Python modules/code
  2. Setup static analysis (you can do this during CI) of the shell scripts and python modules & scripts as part of your quality assurance process
  3. Consider transitioning shell scripts to either Python with robust exception handling or CMake tasks via custom targets and/or ctest tests
    • Python with exception handling is more robust and portable
    • Relying on CMake rather than *nix tools and bash scripts make it more likely to run on all major OSes and the CMake code can be exercised during CI testing or by quickly typing ctest.
  4. Restructure some or all of the documentation, most notably the tutorial, as a Jupyter Notebook.
    • Test the notebook(s) during CI to ensure code changes haven't broken the examples and tutorials
    • This was probably the greatest source of delay and frustration during the review. Automating ways to check the code in the documentation and keeping code with documentation will help ensure things do not get out of sync.

Great job with rescal-snow!

kellykochanski commented 4 years ago

@zbeekman Thank you for the time and attention you have dedicated to this review.

zbeekman commented 4 years ago

@kellykochanski Thanks for asking me to be a reviewer, and for making some really interesting and useful software! I've never seen cellular automata used "for real" before, outside of Conway's Game of Life, so it was very interesting! I am tentatively planning/hoping to submit some pull requests to address some of the next steps outlined above, but I'll wait until after the JOSS article is accepted/published so that you don't have to wait any longer on that. And, obviously, only if it is of interest to you. But setting up CI with Travis-CI would be pretty easy for me.

kellykochanski commented 4 years ago

@zbeekman If you're willing to put a little bit of time into that, I'd be delighted - I looked into CI when you first brought it up (it would have been great to prevent some of the debugging back-and-forth in this review), but it's outside of my current expertise.

Cellular automata have a rather nice history in geoscientific modelling as relatively simple, low-cost approximations for granular physics and fluid dynamics.

I'm trying to avoid relying on Jupyter notebooks, unfortunately - I do production runs in an hpc environment without access to IPython, and the tutorial is used by students learning to work in the same environment.

kellykochanski commented 4 years ago

@kthyng The issues #41 and #44 are now closed, and I believe both reviewers have recommended JOSS acceptance.

kthyng commented 4 years ago

Awesome! I will start the next process then.

kthyng commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

kthyng commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Attempting to check references...
kthyng commented 4 years ago

Paper-related changes:

@openjournals/joss-eics Can anyone tell why in this paper the Marsh et al paper doesn't shorten to "Marsh et al' when referenced but instead displays all the authors? This is not consistent with other bib entries with fewer or more number of authors. And same question for the Narteau reference which displays all author names inline.

kthyng commented 4 years ago

I think there are a few more bib-related errors actually; you can get more details for how to format your references here.

For example, the top two lines of reference Filhol2015 should be as below to get the order of the initial correct in the references list.

@article{Filhol2015, author = "Filhol, S. and Sturm, M.",

Honestly I'm having trouble keeping track, but generally you'll want to exactly follow the examples on that site so that your references show up exactly right. In the references list, the first author should be shown as "Last name, First initial/name", but any subsequent authors should be listed as "First inital/name Last Name".

kthyng commented 4 years ago

This reference (https://iopscience.iop.org/article/10.1088/1367-2630/14/9/093037) has two more authors that have not been listed.

kthyng commented 4 years ago

These two references have the same doi:

Narteau, C., Zhang, D., Rozier, O., & Claudin, P. (2009). Setting the length and time scales of a cellular automaton dune model from the analysis of superimposed bed forms. Journal of Geophysical Research: Earth Surface, 114(f3). doi:10.1002/esp.3479

Rozier, O., & Narteau, C. (2014). A real‐space cellular automaton laboratory. Earth Surface Processes and Landforms, 39(1), 98–109. doi:10.1002/esp.3479

kellykochanski commented 4 years ago

@kthyng Thank you for the careful review and comments.

kellykochanski commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

PDF failed to compile for issue #1699 with the following error:

Error reading bibliography ./paper.bib (line 63, column 3): unexpected "d" expecting space, ",", white space or "}" Error running filter pandoc-citeproc: Filter returned error status 1 Looks like we failed to compile the PDF

kellykochanski commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

kellykochanski commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

kellykochanski commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...