UBC-MDS / software-review

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

Submission: imgtoolpy (Python) #48

Open rita-ni opened 4 years ago

rita-ni commented 4 years ago

Submitting Author: Ruidan Ni (@rita-ni)
Package Name: imgtoolpy One-Line Description of Package: A Python package that is intended to allow users to compress, sharpen and shrink an input image Repository Link: imgtoolpy Version submitted: v0.3.0 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Kenneth Foo (@kfoofw) Reviewer 2: Gaurav Sinha (@sgauravm) Archive: TBD
Version accepted: TBD


Description

Scope

* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see this section of our guidebook.

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements): "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] 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: *Note: Do not submit your package separately to JOSS*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Code of conduct

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

Editor and review templates can be found here

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

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

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

Final approval (post-review)

Estimated hours spent reviewing:

4 to 5 hours

Review Comments

General Compliments!

Room to improve on!

I'm sorry for the long feedback but I thought since you guys put in the effort to develop the package, I should also put in the effort to try it out. Thanks for the opportunity to try your package! It was fun figuring out the different functions. Please let me know if you have any questions on my feedback!

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

Readme requirements The package meets the readme requirements below:

The README should include, from top to bottom:

Functionality

For packages co-submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

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

Final approval (post-review)

Estimated hours spent reviewing:


Review Comments

The coding is neat and it is well commented which makes it easier to review and understand. The test cases has appropriate negative as well as positive test cases. There are few bugs which I noticed and also some suggestions for improvement:

  1. Include a working example in the README.md file with code and its result.You can check this repo for your reference. This will help the user understand how to use your package and can give the glipse of what each function does. It took me some time to understand how to use it.

  2. You have three separate files for each function. So the obvious command import imgtoolpy won't work for your case. The second column name of the usage section is misleading. It would be better if you mention somewhere how to import your function eg.

    from imgtoolpy import compress,sharpen,shrink
  3. Function compress: Some images have more than 3 channels. I was trying one png image with 4 channels and it threw error. Please add some defensive coding for this so that the user understands what is wrong. Or you can just check for more than 3 channels and strip off the 4th channel inside your code that will make your code more generalized. If you add this write one test case for this one.

  4. Function sharpen: Although you wrote the code defensively such that it doesn't accept images having value other than 0 to 1, it makes such a powerful function very limited as most of the images have values between 0 to 255. You can just handle this by dividing your array by 255. So my suggestion would be that instead of throwing exception just divide the array by 255 and that will make your pixel intensity between 0 to 1. I tried this on my machine and it worked properly.

  5. Function sharpen: Your function takes in colored image and returns a sharpened monotone image. So there is no way to see the difference the function is making. If you could return the non sharpened monotone image along with the sharpened monotone image, that will help to make clear distinction of how well your sharpening function is performing.

  6. The docstring of all the functions doesn't have any example. It is a good practice to include example in docstring for all user facing functions.

I really found your package very interesting and it was fun doing the review. I really enjoyed playing around with the function which gave very interesting results. Hope this review can help enhance your package.

franklu2014 commented 4 years ago

Hi @kfoofw and @sgauravm , thank you for the valuable feedback, and we have addressed some of your important comments in release 1.1.0.

More specifically, below are what have been updated:

Thank you again for your time testing our package!

kfoofw commented 4 years ago

Nice new release of your package! I really like the new photos!

Margaret8521 commented 4 years ago

Thank you @kfoofw and @sgauravm 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!