search

ropensci / software-review

rOpenSci Software Peer Review.
287 stars 104 forks source link

pixelclasser: A Package for Classifying Pixels by Colour #406

Closed CarlosRealR closed 3 years ago

CarlosRealR commented 3 years ago

Submitting Author: Carlos Real (https://github.com/CarlosRealR)
Repository: https://github.com/CarlosRealR/pixelclasser Version submitted: 0.5.0 Editor: @ldecicco-USGS Reviewer 1: @qdread Reviewer 2: Archive: TBD
Version accepted: January 28, 2021

Package: pixelclasser
Type: Package
Title: A Package for Classifying Pixels by Colour
Version: 0.5.0
Authors@R: person("Carlos", "Real", email = "carlos.real@usc.es",  role = c("aut", "cre"), comment = c(ORCID = "0000-0002-5433-6728"))
Description: Contains functions to classify the pixels of an image file
    (jpeg or tiff) by its colour. It implements a simple form of the techniques 
    known as Support Vector Machine adapted to this particular problem.
Encoding: UTF-8
License: GPL-3
LazyData: true
RoxygenNote: 7.1.1
Imports:
    graphics,
    grDevices,
    jpeg,
    tiff,
Suggests: 
    knitr,
    rmarkdown,
    testthat
VignetteBuilder: knitr
URL: https://github.com/CarlosRealR/pixelclasser
BugReports: https://github.com/CarlosRealR/pixelclasser

Scope

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

JOSS Options - [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: - (*Do not submit your package separately to JOSS*)
MEE Options - [x] The package is novel and will be of interest to the broad readership of the journal. - [x] The manuscript describing the package is no longer than 3000 words. - [x] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

melvidoni commented 3 years ago

Hello @CarlosRealR, thanks for submitting. We will assign you a handling editor soon. In the meantime, can you please elaborate on the answers to the questions of the Scope section? (in the opening post)

CarlosRealR commented 3 years ago

Hello @CarlosRealR, thanks for submitting. We will assign you a handling editor soon. In the meantime, can you please elaborate on the answers to the questions of the Scope section? (in the opening post)

Hello @melvidoni, done. I hope my answers would be clear enough now.

melvidoni commented 3 years ago

It does, thank you kindly @CarlosRealR. Your Handling Editor will be @ldecicco-USGS.

ldecicco-USGS commented 3 years ago

Hi @CarlosRealR , I'll begin asking around for 2 reviewers and running the automated package checks. Thanks for expanding on the questions above 👍

One thing that would be helpful while I'm asking around for reviewers would be to "Knit" the Readme.Rmd, and then check-in the Readme.md. The main difference is that Github will then render the file. You could copy a few of the examples in the vignette to showcase the main idea of the package in a pretty/rendered way. So, not required, I'm just thinking it might help the potential reviewers decide if they are a good fit for taking on the review.

Also, not sure how responsive my American colleagues are going to be this week. I'd be prepared for a slightly slower response for the next couple of days than maybe usual.

CarlosRealR commented 3 years ago

Hi @ldecicco-USGS , excuse me, but this is the first time I make a package and send software to GitHub and I'm a a bit confused with the procedure and the best way to do things. What do you mean with "check-in the Readme.md"? I would be grateful if you could tell me a package with a readme that way to understand what I have to do exactly.

ldecicco-USGS commented 3 years ago

Once you knit the Rmd file, it will create an md file, so now on your computer there will be 2 files, the "Rmd" one is specific to the "R" programming language, the "md" is a markdown file (more universal). So not only do you need to generate the "md" file, you need to get it up to Github. If you are in RStudio using the "Git" tab, that will mean clicking on the checkbox next to the new file, hitting "commit", writing a little message, then clicking the "Push" button.

Probably "check-in" was the wrong term, sorry! If you are doing it in the Terminal, it would be something like:

git add README.md
git commit  -m "Adding the md file"
git push origin master
CarlosRealR commented 3 years ago

Hi, I expanded the README.rd including some images as you suggested. Thanks for the advice.

CarlosRealR commented 3 years ago

Hi, I expanded the README.rd including some images as you suggested. Thanks for the advice.

CarlosRealR commented 3 years ago

Hi @melvidoni. sorry, but I closed this by error, I do not know how

ldecicco-USGS commented 3 years ago

Just wanted to check in and let you know I'm sending out some emails today. I ran the automatic checks and everything looks good:

goodpractice::gp()
Preparing: covr
Preparing: cyclocomp
Preparing: description
Preparing: lintr
Preparing: namespace
Preparing: rcmdcheck

♥ Yee-haw! Well-made package! Keep up the prime work!
spelling::spell_check_package()
DESCRIPTION does not contain 'Language' field. Defaulting to 'en-US'.
  WORD            FOUND IN
barycenter      pixelclasser.Rmd:85
carlos          pixelclasser.Rd:28
colour          classify_pixels.Rd:16,28
                define_cat.Rd:12,24
                define_rule.Rd:12,15,34,36,48
                pixelclasser.Rd:6,9,11
                plot_pixels.Rd:12,14,16,27,29,30,31,35
                plot_rgb_plane.Rd:17,19,38
                plot_rule.Rd:16,34,37
                read_image.Rd:18
                description:2
                README.md:11,16,67
                README.Rmd:25,27,54
                pixelclasser.Rmd:21,23,25,62,78,85,114,151,184
Colour          title:1
coloured        pixelclasser.Rmd:85
colours         classify_pixels.Rd:10
                define_cat.Rd:47
                plot_pixels.Rd:32
                read_image.Rd:16
                pixelclasser.Rmd:21,66,96
comparator      define_rule.Rd:63
                pixelclasser.Rmd:132
correctedness   pixelclasser.Rmd:121
gb              pixelclasser.Rmd:96
geq             pixelclasser.Rmd:25
incid           classify_pixels.Rd:41
jpeg            pixelclasser.Rd:9
                read_image.Rd:41
                save_classif_image.Rd:26
                description:2
jpg             read_image.Rd:5
                save_classif_image.Rd:30
ldots           pixelclasser.Rmd:27,29
leq             pixelclasser.Rmd:25
mathbf          pixelclasser.Rmd:27,27,29,29
migth           pixelclasser.Rmd:76
mis             pixelclasser.Rmd:161,254
pertenence      README.md:16
                README.Rmd:27
                pixelclasser.Rmd:23
poligons        pixelclasser.Rmd:172
rgb             plot_pixels.Rd:23
                pixelclasser.Rmd:23,62,64,76
subcat          define_cat.Rd:17
subclasses      pixelclasser.Rmd:184,190
usc             pixelclasser.Rd:28

Some of those are not actually mis-spelled, but you might want to check a few.

Please consider adding an rOpenSci review badge -- either via -- rodev::use_review_badge(406).

Badge URL is https://badges.ropensci.org/406_status.svg. Full link via markdown would be:

[![](https://badges.ropensci.org/406_status.svg)](https://github.com/ropensci/software-review/issues/406)
CarlosRealR commented 3 years ago

I have added a language field to description and corrected the true misspellings. I have added the badge too

ldecicco-USGS commented 3 years ago

Sorry for the delay, it's a tough time for asking...thank you so much @CarlosRealR for your patience!

I'm happy to report that @qdread will be one reviewer. I have confidence I'll get another one pretty soon!

ldecicco-USGS commented 3 years ago

OK, we've got 2 reviewers! @superjai agreed to do the second review. We will shoot to have the reviews returned within 3 weeks...which is December 21st, 2020.

qdread commented 3 years ago

Package Review

@ldecicco-USGS and @CarlosRealR, here is my package review! This is my first review for ROpenSci, so please be understanding if I've left anything out or otherwise not followed best reviewer practices. Thanks for the opportunity.

Documentation

The package includes all the following forms of documentation:

(left out the section on JOSS)

Functionality

Estimated hours spent reviewing: 6

Review Comments

This package is essentially a way to walk the user through the process of manually classifying pixels in an image into different categories based on their relative proportions of red, blue, and green. The user does this by plotting the pixels in 2 of the 3 color dimensions, eyeballing the dividing lines between groups of pixels belonging to differently categorized regions of the image, specifying the coordinates of the line, and then repeating until the cloud of points is "tiled" into convex polygons, each of which is assigned to a category. The resulting classification rules could then be applied to other images. Overall, I found that the package generally follows the ROpenSci package guidelines, and that it works as advertised. Nice job! I do have some general/conceptual comments, as well as some technical comments about the code and documentation.

General comments

Vignette

Comments on specific functions

define_rule

Error in cat(x, ..., sep = sep) : 
  argument 1 (type 'list') cannot be handled by 'cat'

plot_rgb_plane

plot_rule

plot_pixels

classify_pixels

Comments on package documentation

ldecicco-USGS commented 3 years ago

Thanks @qdread ! Looks great to me. @CarlosRealR feel free to respond to @qdread 's comments or wait for @superjai

CarlosRealR commented 3 years ago

Thanks @qdread for your favourable opinion and the helpful comments. I am making some of the suggested changes, but I will wait for the comments of the other reviewer to make the bulk of the changes and to send a joint response. I think it is the best way to proceed.

CarlosRealR commented 3 years ago

Hi @ldecicco-USGS, I am asking the editor of Methods in Ecology and Evolution for an extension of the deadline for resubmitting the paper about pixelclasser, as the one they gave us (january 11) is about to expire. I know that the end of a year are busy times for everybody but, do you know approximately when will we receive the comments of the second reviewer? I am considering whether I must negotiate for a 15 day delay or better for a month. I suppose that MEE will not reject my request, but I prefer asking for too much time now than to ask for successive extensions, less work for everybody.

ldecicco-USGS commented 3 years ago

Sorry for the delay! I'll touch base with @superjai and get back to you.

ldecicco-USGS commented 3 years ago

It sounds like we'll have the 2nd review in by the end of the day tomorrow. Hope that helps with the decision on the extension.

ldecicco-USGS commented 3 years ago

I'm submitting a review due to how long this has taken. If the other reviewer adds more thoughts in the future, you can take that into account in your long-term maintenance and development of the package, but for now just be concerned with this review and qdread's above.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Estimated hours spent reviewing: 3

Review Comments

This is a nice package that performs some very useful functions, and includes ways for the user to have fine control over the way they classify images.

The examples all ran on my computer except the ones that explicitly stated they were hypothetical. There were several examples wrapped in dontrun that I'm not sure why you would not want them run. They were relatively fast and useful. I like to keep as many examples and tests in my checks as possible for long-term stability and sustainability.

I ran the workflow described in the vignette on an image I had on my computer. It was exceptionally slow, but eventually things finished. As I walked through the example with my own image, I could see how useful the package is.

All-in-all, handy package. The code itself looked straightforward and did not raise any red flags.

FYI, my goodpractice::gp() output was:

♥ Hooray! Super-excellent package! Keep up the classy work!
CarlosRealR commented 3 years ago

Thanks for your comments @ldecicco-USGS. I have done most of the modifications suggested by the first reviewer yet, but changes to tests and the vignette were waiting. I will send them on Monday or Thursday. May be that the comments of the second reviewer arrive before that.

CarlosRealR commented 3 years ago

Thanks for the helpful comments about the package, @qdread and @ldecicco-USGS .

Before explaining the changes I wish to make some general comments.

The method is true SVM. In papers explaining the technique (Cortes and Vapnik, 1995; Bennet and Campbell, 2000, now cited in the vignette), the examples illustrating the basics of SVM are very similar to what pixelclasser does (they are 2D examples). Linear regression is the simplest technique of the family of regression methods (including multivariate regression) but it is regression nevertheless, even when you only fits the line without making significance tests. With pixelclasser you can do SVM in a manual, simple way, but it remains (bare bones) SVM.

It is true that the method cannot discriminate categories that are not separable (overlapping). In fact, the example in the vignette was selected to show this: first a clean classification and then a dirty one. Therefore, it is not a universal method for image classification, but it will work in many cases and with much less complexity than other methods that are in use. It also gives the user full control and understanding of what is going on, something that complex libraries and canned code don’t do in many cases.

Processing the data can be slow, specially when plotting the pixels. Note that many cameras can produce 20 million points per image (and 100 is not unusual in hi-res cameras). R plotting routines choke with such a lot of data. I do not see an easy solution to this, (apart from using a sample of the pixels, which distorts the cloud of points). Using a fast computer with lots of memory helps, as usual. I have added a warning in the vignette and in the help page of plot_pixels().

These are the changes and additions: • An outline of the workflow was included in the help page of the package (?pixelclasser). • New functions: ◦ place_rule() wraps a call to graphics::locator() and helps in drawing vertical and horizontal lines. define_rule() can be used as in the previous version and also can be combined with place_rule() to create rules easily. This is illustrated in the vignette and in the examples of its help page. ◦ Labelling the rules is now done with an independent function, labelrule(). In this way label colours, for example, can be set independently from the line colour. • The names of the objects are more coherent now, all starting by pixel • All objects have now its print method. ◦ The print method for the pixel_rule objects writes the equation of the line using the colour variables, and without using generic parameters like a and c). ◦ “redundant” has been substituted by “duplicate” in the output of classify_pixels() . • “comparator” has been substituted by “comp.op” • plot_pixels() has now a … argument to pass parameters to the underlying points() function. • The layout of the vignette introduction was improved (I hope). • I left the error in the definition of rule_05 to show later the usefulness of the checks in classify_pixels(), but flagged and explained it on the spot to avoid confusing the reader.

And this is what was not done:

Due to the problem with plotting a lot of points commented above, trying to plot the three axis combinations in the same graph is not a good idea, so I decided to left the plot functions unchanged.

I think that I addressed all the questions raised in the reviews. I have pushed version 0.7 to the repository. It contains all these changes.

qdread commented 3 years ago

@CarlosRealR Wow this looks great! I'm glad my comments were able to improve your package. @ldecicco-USGS am I expected to do a second review?

ldecicco-USGS commented 3 years ago

It does look great! @qdread , we don't expect a full second review, but ideally you could pull down the new changes, verify the developer addressed any of your particular concerns.

qdread commented 3 years ago

OK, I should be able to do that within the next week or so.

qdread commented 3 years ago

Hi @CarlosRealR I just went over the vignette of your revised package and I must say again, great job! 💪 🙇 You really went above and beyond in addressing my comments. I especially appreciate how the print methods now show concise and informative details on each object, and I like the place_rule() and label_rule() functions! I ran the package checks and everything was 💯 .

I just wanted to note a few small things I noticed:

(1) I got this warning on install:

Rd warning: C:/Users/qread/AppData/Local/Temp/RtmpO0Jg7d/R.INSTALL462c7533769d/pixelclasser/man/classify_pixels.Rd:72: file link 'colours' in package 'grDevices' does not exist and so has been treated as a topic

Maybe this is because of a difference between colors and colours? Not sure.

(2) I got this error when running the code chunk in the vignette demonstrating the place_rule() function:

Error in define_rule("rule_07", "g", "b", rp02, ">=") : 
  rule_points must be a list containing two coordinate vectors

That error happened even though rp02 was an object of class rule_points that I created using rp02 <- place_rule("v").

(3) I didn't see contribution guidelines in the documentation and I didn't know if you wanted to include those.

Anyway, I wanted to bring those small things to your attention. But again, great work and I think my job here is done 😁

CarlosRealR commented 3 years ago

Hi @qdread . I am now correcting the errors that you found in define_rule. I was not able to reproduce the install warning in my linux machine but I will try in a Windows one to see if it appears. Please, could you explain me what are these "contribution guidelines" that should be in the documentation. I have searched in "Writing R extensions" and I found nothing. Is this an ROpenSci issue? I am lost here...

qdread commented 3 years ago

Hi @CarlosRealR I don't know if it is a big deal to include contribution guidelines but as you can see in the review checklist under "documentation" there is a checkbox for "community guidelines" potentially including contribution guidelines. Many packages include a file called CONTRIBUTING.md that explains how to contribute to the package. It might be as simple as saying open a pull request on the GitHub repo for the package.

Here are a couple examples

But I don't think it needs to be too complex. I will leave it up to the ROpenSci people to say whether this is something that absolutely needs to be included or not.

CarlosRealR commented 3 years ago

Hi . @ldecicco-USGS and @qdread. I have just pushed the revised version of pixelclasser. I have corrected the errors that @qdread find in the previous version, I finally tracked them all. I changed a little the interface of place_label() and polished the whole set of function help files. In these, I corrected some inconsistencies and some crap from old versions that managed to escape my previous inspection. I also modified some examples which, as @ldecicco-USGS commented, can run without problem and were previously marked as not to run. I also added a very simple CONTRIBUTION file, simply recommending to send an e-mail to me. When the package will be finally accepted, will be the time to add gitHub issues and the like, I think.

ldecicco-USGS commented 3 years ago

Approved! Thanks @CarlosRealR for submitting and @qdread for your review! 😸

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

CarlosRealR commented 3 years ago

Good news! Thanks again for the comments on the package, @ldecicco-USGS and @qdread. They helped to improve it, really. Now I am doing a last review before convert it to the version 1.0. I am including @qdread in the credits as reviewer, but I think that you, @ldecicco-USGS has also done review work here. If you wish, I will include you as reviewer, but perhaps this is not compatible with the role of editor, or redundant. Tell me if this convenient or not, please.

ldecicco-USGS commented 3 years ago

Yeah, no need to include me as a reviewer, thanks for asking though.

CarlosRealR commented 3 years ago

I have a question @ldecicco-USGS I tried to transfer the repo as indicated in your comment. As I understand this, I must use the option Transfer ownership in the Danger Zone of the Settings of my repository, indicating ropensci as the organization owning the new repo. But I receive this message: ropensci/pixelclasser already exists and You don’t have the permission to create public repositories on ropensci

and nothing happens.

I have also tried the git instructions that appear in ropensci/pixelclasser, but these do not work neither. I think that my configuration points to my repo, so I must dig a bit in how to change my configuration. But I am not sure if I am correct or not. Could you give me some tip about this?

ldecicco-USGS commented 3 years ago

Sure enough, I think I messed that up. I took down the empty repo, and made sent you an invitation to a "pixelclasser" team. Once you accept that invitation, try transferring it again. If that doesn't work, let me know.

ldecicco-USGS commented 3 years ago

@CarlosRealR It looks like pixelclasser is all set. I'm going to close the issue, but feel free to re-open if there are lingering issues/questions. Thanks again for the submission!