UBC-MDS / software-review

MDS Software Peer Review of MDS-created packages
1 stars 0 forks source link

Submission: rimgtool (R) #47

Open franklu2014 opened 4 years ago

franklu2014 commented 4 years ago

Submitting Author: Ruidan Ni (@rita-ni), Frank Lu (@franklu2014), Kexin Zhao (@Margaret8521) Repository: rimgtool Version submitted: 0.3.0 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Fanli Zhou (@flizhou) Reviewer 2: Haipeng Zhang (@Zhang-Haipeng) Archive: TBD Version accepted: TBD


Package: rimgtool
Title: Image processing
Version: 0.0.0.9000
Authors@R: 
   c(person(given = "Frank",
           family = "Lu",
           role = c("aut", "cre"),
           email = "franklu@live.ca",
           comment = c(ORCID = "YOUR-ORCID-ID")),
    person(given = "Rita",
           family = "Ni",
           role = c("aut"),
           email = "niruidan@gmail.com"),
    person(given = "Margaret",
           family = "Zhao",
           role = c("aut"),
           email = "margaretzhao@yahoo.com"))
Description: The package can compresses, sharpen, or shrink images.
    The compress function can reduce the byte size of the image, 
    the shrink function reduce image size by removing border pixels, 
    the sharpen function by applying special filters.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.0.2
Suggests: 
    testthat (>= 2.1.0),
    covr,
    knitr,
    rmarkdown
URL: https://github.com/UBC-MDS/rimgtool
BugReports: https://github.com/UBC-MDS/rimgtool/issues
VignetteBuilder: knitr

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 - [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] 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

Zhang-Haipeng commented 4 years ago

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

Final approval (post-review)

Estimated hours spent reviewing:


Review Comments

library(jpeg) # install.packages('jpeg')
library(OpenImageR) # install.packages('OpenImageR')
library(rimgtool) # devtools::install_github("UBC-MDS/rimgtool")

img <- jpeg::readJPEG('img.jpg')
img_compress <- rimgtool::compress(img, 20L)
img_sharpen <- rimgtool::sharpen(img)
img_shrink <- shrink(img, 500, 500)

OpenImageR::writeImage(img_compress, 'img_compress.jpg')
OpenImageR::writeImage(img_sharpen, 'img_sharpen.jpg')
OpenImageR::writeImage(img_shrink, 'img_shrink_500_500.jpg')
franklu2014 commented 4 years ago

Thank you for the feedback. I will respond to your first question and go from there. The idea of this package is for image processing, but we don't want to constrain users from using any image format. Some users might prefer .jepg, like yourself, but there are many other formats might be preferred by other users. That's why we want to leave the flexibility to the users for choosing whatever rendering format they like, and we deal with the core of the image, which is an array.

Just think of Scikit-Learn. Users create their own arrays from .csv, .xlsx, or any sort of input, and the models take in the arrays, do the processing, and return a processed array. This package tries to follow the same procedure as Scikit-Learn and takes in arrays and returns processed arrays.

franklu2014 commented 4 years ago

By the way, I found questions 9 and 10 are contradicting. Since question 10 indicates that an example can be found, questtion 9 is satisfied.

Zhang-Haipeng commented 4 years ago

Thank you for the feedback. I will respond to your first question and go from there. The idea of this package is for image processing, but we don't want to constrain users from using any image format. Some users might prefer .jepg, like yourself, but there are many other formats might be preferred by other users. That's why we want to leave the flexibility to the users for choosing whatever rendering format they like, and we deal with the core of the image, which is an array.

Just think of Scikit-Learn. Users create their own arrays from .csv, .xlsx, or any sort of input, and the models take in the arrays, do the processing, and return a processed array. This package tries to follow the same procedure as Scikit-Learn and takes in arrays and returns processed arrays.

@franklu2014 That's a fair point. But still, there are pros and cons to such a decision. I think it's up to you guys to make the call.

Zhang-Haipeng commented 4 years ago

By the way, I found questions 9 and 10 are contradicting. Since question 10 indicates that an example can be found, questtion 9 is satisfied.

Not really. Because for one thing, what we have is an image, which won't work as helper data for our package for now (since our package requires array). Secondly, there's no instruction to guide the user/tester to use this image as helper data. It's more like a documentation file that we used in developing the package. (I also wonder if a greyscale image could be used for testing compress function.)

flizhou commented 4 years ago

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

Final approval (post-review)

Estimated hours spent reviewing:


Review Comments

I like your ideas about image processing and enjoined using your package. The package is well written and well organized overall. Just in case it would help, I want to share with you my experience/thoughts while using your package.

  1. New to this package, I was confused about what to pass to your functions at first. I later saw a code example in your vignette page. I would recommend to include examples in your README as well so that they are more noticeable.

  2. The description and inputs of the compress function confused me since I'm not familiar with how images are stored. It would be helpful to give an example showing the input and output for this function.

  3. The shrink function is quite unexpected as it has the name 'shrink' but trims the image instead. I would recommend either change the function name or change its functionality.

  4. The shrink function requires users to pass width and height values that are smaller than those of the original image, but usually users such as myself may forget to check the size of the image. To make life easier, I would recommend switch to proportions as your inputs for desired width and height.

  5. Sorry to be picky, but your README.Rmd and README.md files do not match. I think it would be a good practice to modify your README.Rmd and knit it to get an update-to-date README.md. One advantage of using README.Rmd is that it allows you to show and run code in your README.

  6. In the package overview part of your README, you may intend to say vector but said 'numpy array' instead.

  7. You may want to update your dependencies in README, since the information there does not match with your DESCRIPTION file.

Margaret8521 commented 4 years ago

Thank you @Zhang-Haipeng and @flizhou for your valuable feedback. We were unable to address all your feedback due to time constraints, but here is a list of what we improved:

The feedback was extremely helpful, thank you again! You can check our new release here.