UBC-MDS / software-review

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

Submission: Rmagine #17

Open katieb1 opened 4 years ago

katieb1 commented 4 years ago

Submitting Author: Katie Birchard (@katieb1), Brendon Campbell (@brendoncampbell), Sukriti Trehan (@Sukriti1312), Trevor Kwan (@trevorkwan) Package Name: Rmagine One-Line Description of Package: Implements image filtering in R Repository Link: https://github.com/UBC-MDS/Rmagine Version submitted: 1.1.0 Editor: @kvarada
Reviewer 1: @sreejithmunthikodu
Reviewer 2: @merveshin Archive: TBD
Version accepted: TBD


Description

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

sreejithmunthikodu 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:

sreejithmunthikodu commented 4 years ago

Review Comments

Installation:

From vignette("Rmagine"):

Warning message:
vignette ‘Rmagine’ not found 

From help():

From test():

From tunnel_filter() :

From colour_filters():

From edge_detection():

From vignette_filter():

merveshin 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: 4 hours


Review Comments

In general, I like the functions you implemented. They are easy to understand and use. The documentation is sufficient to start playing with. The structure inside the functions is clear and understandable. The code was clear. Well done.

However, I have a few improvement ideas. I will group them to follow easier:

Documentation and Structure:

In ReadMe, usage information for vignette_filter function is good and helpful to onboard using this package. It could be even better if you put more examples to cover the other three functions.

Usability & Clarity

Some code lines are repeating across functions such as input validation and some lines are repeating within the function like image save statement in color_filter function. Reusing code and referring to one function would make it easier to maintain, test and change in the future. This redundancy applies to test functions too. If you can create a separate function to validate input, the repeatation in test files also can be removed.

For all four functions, it would be better if you give the user an option to pick the destination folder and file name.

For color_filter function, we specify red, green or blue tone filter, I would like to have control over the strength as well. This is just a suggestion, I think this kind of improvements can increase adoption of the library.

Testing

Unit tests are good and covering all lines. Although there are enough test cases for negative tests, the positive test case is not covering what is intended to be tested. There should be at least one happy path test where you run your function and compare it with the expected output. I suggest you store expected images in img folder and check if generated files are actually the same as them. I saw that only image sizes are compared.

Finally, this is more like a question, in all of the functions, you assumed as.array(image) statement will read always RBG image. Is it possible that the given image is already grayscale and looking for the other layer throw an error?

Sukriti1312 commented 4 years ago

@sreejithmunthikodu Thanks for the feedback! Here are the suggested changes that we have made:

Link for the latest release with incorporated changes

Sukriti1312 commented 4 years ago

@merveshin Thank you for your feedback! Following are the changes that we have made to our package as per your suggestion:

Link for the latest release with incorporated changes