Joss-reviews: [REVIEW]: pymccrgb: Color- and curvature-based classification of multispectral point clouds in Python

Created on 2 Oct 2019  Β·  77Comments  Β·  Source: openjournals/joss-reviews

Submitting author: @rmsare (Robert Sare)
Repository: https://github.com/rmsare/pymccrgb
Version: v0.1.6
Editor: @kthyng
Reviewer: @martibosch, @daniellivingston
Archive: 10.5281/zenodo.3519582

Status

status

Status badge code:

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

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

@martibosch & @daniellivingston, 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 @martibosch

Conflict of interest

  • [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • [x] Repository: Is the source code for this software available at the repository url?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@rmsare) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation?
  • [x] Functionality: Have the functional claims of the software been confirmed?
  • [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @daniellivingston

Conflict of interest

  • [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • [x] Repository: Is the source code for this software available at the repository url?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@rmsare) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation?
  • [x] Functionality: Have the functional claims of the software been confirmed?
  • [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
accepted published recommend-accept review

All 77 comments

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @martibosch, @daniellivingston 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
Attempting PDF compilation. Reticulating splines etc...

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

Can't find any papers to compile :-(

@whedon commands

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

Oops, the repo URL should be https://github.com/rmsare/pymccrgb. @kthyng, can this be changed, or should I withdrawn/resubmit with the corrected URL?

I just edited the first comment in this issue to update all of the URLs (I think). Let me know if you find one with the old one.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

Thanks, looks great!

cc'ing @gehilley for visibility

I wanted to note that one reference (Pedregosa, et al., 2011, JMLR) does not have a DOI.

It appears that the journal did not assign it a DOI as it's not in CrossRef. The arxiv version of this paper has the same issue.

Hi @kthyng , it appears that I haven't been formally assigned to this review yet.

@arfon Is whedon sometimes slow about assignments? In this case it would be days slow...

@daniellivingston now looks like you are set.

@rmsare , on x86_64 Ubuntu 18.04.1, running the command:

$ sudo apt-get install -y liblas-c-dev

gives the error:

404 Not Found

E: Aborting install.

Trying to run conda env create -f environment.yml fails due to liblas/writer.hpp not being found.

EDIT: Sorry, ignore this. I'm running in a VirtualBox and did not sudo apt-get update before trying to install liblas.

@rmsare , I have completed my review. Great package that may find a use within my research group. I am recommending your submission for publication.

On your ReadTheDocs documentation, the left sidebar has an area where the background is pure white, obstructing the package name. This is a minor thing and does not impact legibility of the documentation, but should be addressed.

@daniellivingston Thanks. Unfortunately the installation is a little finicky, but I'm glad it worked in your VM. If there are any other issues I'd be happy to update the packaging.

I've fixed the header color so the title is now readable on https://pymccrgb.readthedocs.io/en/latest/

Hi @martibosch can you work on your review here soon?

Hello, sorry for the delay. Here I go:

  • Contribution and authorship: I noticed from the repository that all code has been written by @rmsare (except a very small commit by the second author). Of course there are many potential significant contributions beyond the code itself. I am just noting this to ensure that the author list complies with JOSS authorship criteria.

  • Installation: while I could successfully install the package by following the instructions of the documentation, I believe that the procedure is a bit too convoluted, since it requires cloning the source repository just for the environment file and then suggests to install the package from PyPI when, given that the repository has already been cloned, the package could be installed as in pip install .. I would strongly suggest the authors to create a conda recipe so that the package con be installed from conda-forge without the need for cloning the source repository. Finally, I wonder whether it is essential to include a 21.6 MB example data which practically accounts for the totality of the size of the Python wheel or source tar. I believe that it would be better to include such example data in a separate repository (e.g., with the example notebooks there instead of in docs/source/examples) or hosted on an s3 instance so they are downloaded (and maybe cached locally) once the users call the load_ methods of the dataset module.

  • Automated tests: the tests of the repo are satisfactory. I would further suggest that the authors report the test coverage to a service such as coveralls. But this is just a personal suggestion.

  • References: as noted above by @rmsare , the reference for scikit-learn does not have a DOI. As a reviewer, should I put a tick to the checklist anyway, @kthyng ?

A final side note: I believe that a linter such as flake8 should be used in order to prevent bugs and conform to Python standards. From the root of the repository (commit d97450d), the call to flake8 returned the following (note that line ./pymccrgb/pointutils.py:28 of crop_to_polygon would raise a NameError):

./setup.py:11:80: E501 line too long (92 > 79 characters)
./setup.py:38:80: E501 line too long (80 > 79 characters)
./docs/source/conf.py:58:1: E302 expected 2 blank lines, found 1
./docs/source/conf.py:86:1: E402 module level import not at top of file
./docs/source/conf.py:87:1: E402 module level import not at top of file
./docs/source/conf.py:156:1: E402 module level import not at top of file
./pymccrgb/core.py:90:1: W293 blank line contains whitespace
./pymccrgb/core.py:94:1: W293 blank line contains whitespace
./pymccrgb/core.py:103:1: W293 blank line contains whitespace
./pymccrgb/core.py:205:72: W291 trailing whitespace
./pymccrgb/core.py:211:22: W291 trailing whitespace
./pymccrgb/core.py:222:1: W293 blank line contains whitespace
./pymccrgb/core.py:318:80: E501 line too long (87 > 79 characters)
./pymccrgb/core.py:339:80: E501 line too long (83 > 79 characters)
./pymccrgb/__init__.py:1:1: F401 '.core' imported but unused
./pymccrgb/__init__.py:1:1: F401 '.datasets' imported but unused
./pymccrgb/__init__.py:1:1: F401 '.features' imported but unused
./pymccrgb/__init__.py:1:1: F401 '.ioutils' imported but unused
./pymccrgb/__init__.py:1:1: F401 '.pointutils' imported but unused
./pymccrgb/__init__.py:1:1: F401 '.plotting' imported but unused
./pymccrgb/__init__.py:2:1: F401 '.core.mcc' imported but unused
./pymccrgb/__init__.py:2:1: F401 '.core.mcc_rgb' imported but unused
./pymccrgb/__init__.py:3:1: F401 '.ioutils.read_data' imported but unused
./pymccrgb/__init__.py:3:31: W291 trailing whitespace
./pymccrgb/ioutils.py:10:15: E225 missing whitespace around operator
./pymccrgb/ioutils.py:12:1: E302 expected 2 blank lines, found 1
./pymccrgb/ioutils.py:39:80: E501 line too long (80 > 79 characters)
./pymccrgb/ioutils.py:43:80: E501 line too long (80 > 79 characters)
./pymccrgb/ioutils.py:46:80: E501 line too long (100 > 79 characters)
./pymccrgb/ioutils.py:51:80: E501 line too long (81 > 79 characters)
./pymccrgb/ioutils.py:63:22: W291 trailing whitespace
./pymccrgb/ioutils.py:129:5: F841 local variable 'count' is assigned to but never used
./pymccrgb/ioutils.py:159:80: E501 line too long (117 > 79 characters)
./pymccrgb/ioutils.py:160:9: E131 continuation line unaligned for hanging indent
./pymccrgb/ioutils.py:170:5: F841 local variable 'count' is assigned to but never used
./pymccrgb/ioutils.py:183:80: E501 line too long (88 > 79 characters)
./pymccrgb/ioutils.py:184:9: E131 continuation line unaligned for hanging indent
./pymccrgb/ioutils.py:192:5: F841 local variable 'count' is assigned to but never used
./pymccrgb/pointutils.py:5:1: F401 'pdal' imported but unused
./pymccrgb/pointutils.py:28:27: F821 undefined name 'poly'
./pymccrgb/pointutils.py:78:1: W293 blank line contains whitespace
./pymccrgb/pointutils.py:90:80: E501 line too long (111 > 79 characters)
./pymccrgb/pointutils.py:113:80: E501 line too long (104 > 79 characters)
./pymccrgb/pointutils.py:149:80: E501 line too long (99 > 79 characters)
./pymccrgb/classification.py:36:22: W291 trailing whitespace
./pymccrgb/classification.py:50:80: E501 line too long (88 > 79 characters)
./pymccrgb/colorize.py:3:1: F401 'pdal' imported but unused
./pymccrgb/plotting.py:10:1: F401 'mpl_toolkits.mplot3d.Axes3D' imported but unused
./pymccrgb/plotting.py:75:80: E501 line too long (83 > 79 characters)
./pymccrgb/plotting.py:110:80: E501 line too long (80 > 79 characters)
./pymccrgb/plotting.py:189:80: E501 line too long (83 > 79 characters)
./pymccrgb/plotting.py:196:80: E501 line too long (81 > 79 characters)
./pymccrgb/plotting.py:202:12: F821 undefined name 'updated'
./pymccrgb/plotting.py:205:5: F841 local variable 'ymax' is assigned to but never used
./pymccrgb/plotting.py:207:80: E501 line too long (81 > 79 characters)
./pymccrgb/datasets.py:46:80: E501 line too long (92 > 79 characters)
./pymccrgb/tests/test_features.py:5:1: F401 'pytest' imported but unused
./pymccrgb/tests/test_features.py:29:80: E501 line too long (80 > 79 characters)
./pymccrgb/tests/test_features.py:44:80: E501 line too long (83 > 79 characters)
./pymccrgb/tests/test_features.py:51:80: E501 line too long (81 > 79 characters)
./pymccrgb/tests/context.py:4:80: E501 line too long (85 > 79 characters)
./pymccrgb/tests/context.py:6:1: E402 module level import not at top of file
./pymccrgb/tests/context.py:6:1: F401 'pymccrgb' imported but unused
./pymccrgb/tests/test_core.py:5:1: F401 'pytest' imported but unused
./pymccrgb/tests/test_core.py:29:80: E501 line too long (83 > 79 characters)
./pymccrgb/tests/test_core.py:35:80: E501 line too long (99 > 79 characters)
./pymccrgb/tests/test_core.py:94:80: E501 line too long (87 > 79 characters)

I believe thits is my review. Very cool package, which I recommend for publishing after some of my minor reviews are adressed. I will most likely use it in the context of a tree detection library that I am working on.

Cheers,
MartΓ­

@martibosch Thanks for your detailed review. Just to give myself a TODO list, here is what I think needs to be addressed:

  • [ ] Improve installation, possibly with conda recipe
  • [x] Remove example data from package, but download it when datasets.load_... is first called
  • [x] Fix flake8 failures and run a linter
  • [x] Sort out the DOI issue

Please let me know if there's anything I've missed. I will work on these soon and ping you all in this thread when I'm done.

I'm glad to hear you think this package will be useful to your work, too!

... and to clarify authorship, the second author @gehilley has supported development and some related data acquisition. (He is my PhD supervisor.)

Code-wise, he is responsible for the wrapper this package depends on, which is hosted separately at https://github.com/stgl/pymcc

Hello @rmsare . Your TODO list seems a great response to my review. I will stay tuned.

Regarding the authorship, I did not intend to cast any doubt about the contributions made by second author, I just wanted to make sure that it is in line with the JOSS guidelines.

@martibosch I've addressed your comments. Please let me know if my changes are sufficient - there are more details in the comments to each review issue.

Re: installation, it will take some time to get the package up on conda forge. However, I've outlined the planned changes to the install instructions and will comment/close that issue when the recipe is accepted.

@kthyng How do you think we should handle the DOI-less citation? It looks like that journal hasn't consistently given DOIs to its articles.

As a reviewer, should I put a tick to the checklist anyway

@martibosch Yes please check it if all available doi's have been included.

Hello @rmsare @kthyng !

I believe @rmsare has addressed all my comments. I have checked all the boxes of the checklist. Everything is good from my side :tada:

Thanks @martibosch!

From what I can see, the reviewers comments have been addressed. It looks like @rmsare is working on getting the package into conda-forge, is that right?

@kthyng Yep. A dependency, liblas, has several conda recipes on different channels but no conda-forge package. I'm trying to add a minimal recipe for that as well which is delaying things a bit.

@rmsare Ok great, ping me when you're finished.

@kthyng The conda package is nearly there and I've made functional recipes to install the two major dependencies (libLAS and pymcc). However there are some conflicts in the pymccrgb recipe that will take some time to fix.

MartΓ­ Bosch agreed that we can proceed with the review without a finalized conda package. I am working to debug the recipe with conda-forge CI and hopefully this won't take more than a week or two to resolve. Sorry about the difficulties here.

I've archived the final version (0.1.6) at 10.5281/zenodo.3519582

@rmsare Ok makes sense. We can move forward then.

Please edit the metadata in your zenodo archive so that the title and author list match your paper exactly.

Also can I verify that v0.1.6 is the version that should be archived here?

Great, thank you! I updated the title and author list on Zenodo.

Yes, 0.1.6 is the version that should be archived with the paper.

Oops I see the version was verified in your previous message, sorry about that.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@whedon check references

Attempting to check references...

@rmsare Can you fix the capitalization in your bibtex entries? To preserve capitalization from the .bib file to the resulting pdf, you can use {} around what you want to preserve.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@kthyng The title and journal capitalization should be fixed now

@whedon set 10.5281/zenodo.3519582 as archive

OK. 10.5281/zenodo.3519582 is the archive.

@whedon set v0.1.6 as version

OK. v0.1.6 is the version.

@whedon accept

Attempting dry run of processing paper acceptance...

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

E, [2019-10-30 13:59:55#28] ERROR -- : Failed to parse BibTeX on value "," (COMMA) [#whedon:116:in <top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:inload'
from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in `

'

@rmsare Looks like it is borking on the scikit learn bib entry. You have an empty doi followed by a comma. The last line in a bib entry shouldn't have a comma, and perhaps you can leave out the doi line altogether since there isn't one? Then remove the comma from the line above it.

Also just know that I am ready to accept your submission once this goes through, so be sure it looks perfect to you.

Yep, this PR should fix this: https://github.com/rmsare/pymccrgb/pull/21

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

Thank you for troubleshooting my Bibtex, and sorry for my delay - was travelling without web access.

I've accepted the empty doi PR and fixed two reference issues. The proof looks good to me.

```Reference check summary:

OK DOIs

  • 10.1109/TGRS.2006.890412 is OK
  • 10.1109/MCSE.2011.37 is OK
  • 10.5281/zenodo.2556738 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

```Reference check summary:

OK DOIs

  • 10.1109/TGRS.2006.890412 is OK
  • 10.1109/MCSE.2011.37 is OK
  • 10.5281/zenodo.2556738 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

@whedon accept

Attempting dry run of processing paper acceptance...

```Reference check summary:

OK DOIs

  • 10.1109/TGRS.2006.890412 is OK
  • 10.1109/MCSE.2011.37 is OK
  • 10.5281/zenodo.2556738 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

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

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1067, 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:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/1068
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01777
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! πŸŽ‰πŸŒˆπŸ¦„πŸ’ƒπŸ‘»πŸ€˜

    Any issues? notify your editorial technical team...

@martibosch, @daniellivingston - many thanks for your reviews here and to @kthyng for editing this submission ✨

@rmsare - your paper is now accepted into JOSS :zap::rocket::boom:

: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:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.01777/status.svg)](https://doi.org/10.21105/joss.01777)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01777">
  <img src="https://joss.theoj.org/papers/10.21105/joss.01777/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.01777/status.svg
   :target: https://doi.org/10.21105/joss.01777

This is how it will look in your documentation:

DOI

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:

Thank you for the editorial help and to @daniellivingston and @martibosch for their reviews.

I'll update/close the install issue once the conda package is accepted.

Was this page helpful?
0 / 5 - 0 ratings