search

openjournals / joss-reviews

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

[REVIEW]: lidar: A Python package for delineating nested surface depressions from digital elevation data #2965

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @giswqs (Qiusheng Wu) Repository: https://github.com/giswqs/lidar Version: 0.6.1 Editor: @kbarnhart Reviewers: @laijingtao, @cheginit, @amanaster2 Archive: 10.5281/zenodo.4571011

: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/005bf3e6f47840e74e71678d8e88facc"><img src="https://joss.theoj.org/papers/005bf3e6f47840e74e71678d8e88facc/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/005bf3e6f47840e74e71678d8e88facc/status.svg)](https://joss.theoj.org/papers/005bf3e6f47840e74e71678d8e88facc)

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

@laijingtao & @cheginit & @amanaster2, 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 @kbarnhart know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @laijingtao

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @cheginit

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @amanaster2

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @laijingtao, @cheginit, @amanaster2 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 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cageo.2005.11.002 is OK
- 10.1016/j.geomorph.2016.05.006 is OK
- 10.1007/s13157-015-0731-6 is OK
- 10.1080/13658816.2015.1038719 is OK
- 10.1111/1752-1688.12689 is OK
- 10.1080/13658810500433453 is OK
- 10.1080/13658816.2014.975715 is OK
- 10.1002/hyp.10648 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.7717/peerj.453 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.7287/peerj.preprints.27099v1 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

kbarnhart commented 3 years ago

It looks like I accidentally unassigned the reviewers so I'm going to re-add them with whedon.

kbarnhart commented 3 years ago

@whedon add @laijingtao as reviewer

whedon commented 3 years ago

OK, @laijingtao is now a reviewer

kbarnhart commented 3 years ago

@whedon add @cheginit as reviewer

whedon commented 3 years ago

OK, @cheginit is now a reviewer

kbarnhart commented 3 years ago

@whedon add @amanaster2 as reviewer

whedon commented 3 years ago

OK, @amanaster2 is now a reviewer

laijingtao commented 3 years ago

Hi Katy @kbarnhart, it seems that I can't click the checkbox and my names is not listed as one of the assignees. Also, I clicked the link for accepting invitation but it said that it could find the invitation. This is my first time reviewing for joss. Could you help me figure out what could be wrong?

kbarnhart commented 3 years ago

@whedon re-invite @laijingtao as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@laijingtao please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

kbarnhart commented 3 years ago

@laijingtao it should work after you click the above link. If it doesn't let me know.

whedon commented 3 years ago

:wave: @laijingtao, please update us on how your review is going.

whedon commented 3 years ago

:wave: @amanaster2, please update us on how your review is going.

whedon commented 3 years ago

:wave: @cheginit, please update us on how your review is going.

kbarnhart commented 3 years ago

@laijingtao, @amanaster2, @cheginit, we have an automatic reminder two weeks into the review. Feel free to provide an update if you have one.

laijingtao commented 3 years ago

I've finished my review for this repository. This package meets all the review criteria and is suitable for publication. My only suggestion is the documentation of the core methods could be improved by adding detailed description and example inputs/outputs. Other parts of the documentation are all great.

cheginit commented 3 years ago

I've been reviewing your submission and here are my comments so far:

  1. The article reads well and states the package capabilities clearly. However, you wrote on line 6:

lidar is a Python package for terrain and hydrological analysis using digital elevation models (DEMs).

Then on line 18 said:

The lidar Python package implements the level-set method and makes it possible for delineating the nested hierarchy of surface depressions as well as elevated terrain features.

The first part makes the scope of applicability of the package very broad, terrain and hydrological analysis, and then second part says that the package mainly implements level-set method for delineating depressions. So I think you need to make the scope of applicability more clear in the summary part similar to the "Statement of Need" section where you correctly state that the package does some terrain analysis which can be used with other hydrology analysis tools.

  1. Regarding the tests, I ran coverage and here is the result:
$ coverage run --source lidar -m unittest discover tests/
$ coverage report -m
Name                 Stmts   Miss  Cover   Missing
--------------------------------------------------
lidar/__init__.py        9      0   100%
lidar/cli.py             9      3    67%   11-13
lidar/example.py        29     29     0%   1-48
lidar/filling.py       151     12    92%   119, 274, 356-376
lidar/filtering.py      85     36    58%   54-56, 84-86, 114-116, 125-175
lidar/gui.py            62     50    19%   15-165, 169, 173, 177
lidar/lidar.py           1      1     0%   3
lidar/mounts.py         55     20    64%   54, 72, 102-133
lidar/slicing.py       375     49    87%   211-216, 262-287, 320, 363, 494-497, 598, 600, 705-722
--------------------------------------------------
TOTAL                  776    200    74%

Though not necessary, my suggestion is to add a coverage badge and try to increase the coverage percentage to around 90% at least.

  1. I am not sure about the journal's policies regarding code quality, @kbarnhart, but, in my opinion, the code needs some polishing. Some files have if-main as well as commented out lines of code which shouldn't be in the published version of a code. If these kind of things matter, I can provide a more detailed list of things that needs to be addressed.
  2. Regarding the documentation, I think the usage section is lacking. The lidar section is essentially a duplicate of all the other sections. The history section is empty as well.
kbarnhart commented 3 years ago

@cheginit thanks for these thoughtful comments. @giswqs when you respond to them, please paste them into an in-repo issue and comment there. Include a link to this repo by including the text openjournals/joss-reviews/issues/2965. This keeps only the top-level discussion on this thread, and more detailed discussion on in-repo issue threads and links those issues with this one.

I broadly agree with @cheginit 's comments and will expand a bit on items 2 and 3. If further clarification from me is useful on any of these issues, just tag me here or in the in-repo issue.

  1. My only addition regarding tests is to emphasize the distinction between tests of interface (e.g., if the right format input is provided, can it run without errors) and tests that verify the algorithms are behaving as expecting). My experience is that it is not too hard to create 100% code coverage that is just a test of the interface but much harder to reach a lower coverage level with algorithm verification. Both types of tests are important and having tests that verify the algorithms are implemented correctly are critical.

  2. JOSS doesn't have any explicit statements about code quality. However, we do expect

    In addition, JOSS requires that software should be feature-complete (i.e. no half-baked solutions) and designed for maintainable extension (not one-off modifications of existing tools). “Minor utility” packages, including “thin” API clients, and single-function packages are not acceptable.

    To that end, a user shouldn't be expected to modify the source code in order to use it to its full level of functionality. E.g., modifying the if-main block to use a different parameter value or input file. I don't think if-main blocks are inherently problematic, but if they exist, a user shouldn't need to edit the code in order to use them. And the functionality of using that if-main block by running it as a script rather than importing its contents should be documented.

giswqs commented 3 years ago

@kbarnhart @cheginit @laijingtao Thank you all for your very helpful comments. I have opened an issue on the lidar repo. I will keep you posted on this thread once I resolve all the comments. Thanks.

kbarnhart commented 3 years ago

@giswqs thanks for the update and for linking it to this issue 🎉 . If you have questions for me at any point in this process, please don't hesitate to ask.

amanaster2 commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

giswqs commented 3 years ago

Not sure why it fails to compile. I just tested it using https://whedon.theoj.org and it complied just fine.

giswqs commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

giswqs commented 3 years ago

I have re-built the documentation using mkdocs. Here is the new link (https://lidar.gishub.org). I am still updating the content. The lidar.reathedocs.io is no longer being updated. Thanks.

cheginit commented 3 years ago

@giswqs Thanks for taking the time and addressing the issues that I raised. @kbarnhart, in my opinion, the package meets all the criteria.

@giswqs As a side note, I would suggest you to take a look at pre-commit. It's a fantastic tool that can help with checking the quality of your code. It can automate many of the tasks, here's an example config file that I use for my package: https://github.com/cheginit/hydrodata/blob/master/.pre-commit-config.yaml

giswqs commented 3 years ago

@cheginit Many thanks for your recommendation of pre-commit. It seems a very useful tool. I will start using it in my projects.

kbarnhart commented 3 years ago

@laijingtao and @cheginit thanks much for your thoughtful reviews and thanks to @giswqs for addressing their issues so expediently, this often makes the effort of subsequent reviewers much more straightforward.

@amanaster2 thank you for being the reviewer with the Arc license. As you are able, feel free to update me on the progress of the review here.

amanaster2 commented 3 years ago

@kbarnhart Will do! I've really only scratched the surface so far. Apologies for the delay... Planning to dig into this further this weekend 😄

kbarnhart commented 3 years ago

@amanaster2 no worries. We are only 4 weeks into the preferred 6 week window. Thanks again for contributing a review, and for testing the ArcGIS components of this submission.

giswqs commented 3 years ago

@amanaster2 I will make a video tutorial for using the ArcGIS toolbox. Hopefully, it will make your review much easier. Thanks.

amanaster2 commented 3 years ago

@kbarnhart Absolutely---happy to do it!

@giswqs That will be excellent. Thank you!

giswqs commented 3 years ago

@amanaster2 I just posted the first video tutorial for using the ArcGIS toolbox. More to come.

https://lidar.gishub.org/get-started/#arcgis-toolbox

giswqs commented 3 years ago

@amanaster2 I just posted another video tutorial for using the toolbox with ArcGIS Pro. Hope it will make it easier for you to test the toolbox. Let me know if you have any questions. Thanks.

https://lidar.gishub.org/get-started/#arcgis-toolbox

amanaster2 commented 3 years ago

Hello @kbarnhart, @giswqs:

I've finished my review. Overall, this is a great piece of software, and I really enjoyed learning how to use it. I have a few comments/suggestions:

  1. Documentation: The installation instructions could use clarification. Please see my comment on this issue for more details.
  2. Documentation: Suggest providing information about the ArcGIS toolbox in the lidar repository. I wouldn't have known there was an ArcGIS toolbox at first glance had @kbarnhart not told me. The tutorials you provided were excellent, and the toolbox works great!
  3. Paper: A little more information regarding the state of the field would be ideal. Are there other software packages that are comparable?
  4. Paper: Update the paper such that the readthedocs isn't listed under Help Documentation and instead list the gishub page.

Please let me know if you have any questions about any of my comments. Otherwise, this software meets all review criteria, and I would be happy to see it published!

giswqs commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

giswqs commented 3 years ago

@amanaster2 Thank you very much for taking the time to review the software and providing feedback. I have updated the repo according to your comments. Thanks.

  1. Improved the installation instructions. See https://lidar.gishub.org/installation
  2. Added the ArcGIS toolbox info. See https://lidar.gishub.org/get-started/#arcgis-toolbox
  3. Added a State of the field section to the paper. See the article proof.
  4. Updated the Help Documentation of the paper. See the article proof.

@kbarnhart Please note that I changed the paper title to lidar: A Python package for delineating nested surface depressions from digital elevation data based on cheginit's comments.

kbarnhart commented 3 years ago

@giswqs thanks for making changes in response to all of the reviewer's comments.

@laijingtao @cheginit @amanaster2 - if @giswqs has addressed all of your concerns and you think that this submission is ready to be accepted, could you please indicate so here. If not, please let me know what else needs to be addressed.

@giswqs , after all reviewer's have recommended the submission be accepted, there are a couple of steps I complete (e.g., copyediting) before we move to the final steps of the JOSS process.

cheginit commented 3 years ago

@kbarnhart All my concerns have been addressed.

amanaster2 commented 3 years ago

@kbarnhart All of my concerns have been addressed. This submission is ready to be accepted!

Thank you for your work, @giswqs.

laijingtao commented 3 years ago

@kbarnhart All my concerns have been addressed

kbarnhart commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.cageo.2005.11.002 is OK
- 10.1016/j.geomorph.2016.05.006 is OK
- 10.1007/s13157-015-0731-6 is OK
- 10.1080/13658816.2015.1038719 is OK
- 10.1111/1752-1688.12689 is OK
- 10.1080/13658810500433453 is OK
- 10.1080/13658816.2014.975715 is OK
- 10.1002/hyp.10648 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.7717/peerj.453 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.7287/peerj.preprints.27099v1 is OK

MISSING DOIs

- None

INVALID DOIs

- None
kbarnhart commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: