-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: pymccrgb: Color- and curvature-based classification of multispectral point clouds in Python #1777
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 🎉. ⭐ Important ⭐ 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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
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:
|
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 |
|
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:
gives the error:
Trying to run EDIT: Sorry, ignore this. I'm running in a VirtualBox and did not |
@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:
A final side note: I believe that a linter such as ./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, |
@martibosch Thanks for your detailed review. Just to give myself a TODO list, here is what I think needs to be addressed:
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. |
|
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) [#<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", "="] |
@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: stgl/pymccrgb#21 |
@whedon generate pdf |
|
@whedon generate pdf |
|
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. |
|
1 similar comment
|
@whedon accept |
|
|
Check final proof 👉 openjournals/joss-papers#1067 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1067, then you can now move forward with accepting the submission by compiling again with the flag
|
@whedon accept deposit=true |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
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 ⚡🚀💥 |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
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. |
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 badge code:
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:
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
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @daniellivingston
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: