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 badge code:
HTML: <a href="https://joss.theoj.org/papers/5d3a09d2d9f94ccc25fcea8667fef6b7"><img src="https://joss.theoj.org/papers/5d3a09d2d9f94ccc25fcea8667fef6b7/status.svg"></a>
Markdown: [](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.)
@martibosch & @daniellivingston, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
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 β¨
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:


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:
datasets.load_... is first calledflake8 failures and run a linterPlease 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) [#on_error': Failed to parse BibTeX on value "," (COMMA) [#<BibTeX::Bibliography data=[1]>, "@", #<BibTeX::Entry >, {:title=>["Scikit-learn: Machine learning in {Python}"], :author=>["Pedregosa, Fabian and Varoquaux, Ga{\\\"e}l and Gramfort, Alexandre and Michel, Vincent and Thirion, Bertrand and Grisel, Olivier and Blondel, Mathieu and Prettenhofer, Peter and Weiss, Ron and Dubourg, Vincent and others"], :journal=>["Journal of Machine Learning Research"], :volume=>["12"], :number=>["Oct"], :pages=>["2825--2830"], :year=>["2011"]}, ",", "doi", "="] (BibTeX::ParseError)
from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/racc/parser.rb:259:in_racc_do_parse_c'
from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/racc/parser.rb:259:in do_parse'
from bibtex.y:111:inparse'
from /app/vendor/bundle/ruby/2.4.0/gems/bibtex-ruby-5.0.0/lib/bibtex/bibliography.rb:67:in parse'
from /app/vendor/bundle/ruby/2.4.0/gems/bibtex-ruby-5.0.0/lib/bibtex/bibliography.rb:50:inopen'
from /app/vendor/bundle/ruby/2.4.0/gems/bibtex-ruby-5.0.0/lib/bibtex/utilities.rb:25:in open'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon/bibtex_parser.rb:36:ingenerate_citations'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon/compilers.rb:222:in crossref_from_markdown'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon/compilers.rb:20:ingenerate_crossref'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/lib/whedon/processor.rb:95:in compile'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/bin/whedon:79:incompile'
from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run'
from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command'
from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch'
from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-74bc29a6a731/bin/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
MISSING DOIs
INVALID DOIs
```Reference check summary:
OK DOIs
MISSING DOIs
INVALID DOIs
@whedon accept
Attempting dry run of processing paper acceptance...
```Reference check summary:
OK DOIs
MISSING DOIs
INVALID DOIs
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:
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:
[](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:
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.