UBC-MDS / software-review-2024

0 stars 0 forks source link

VisuMorph - Group 1 #18

Open SoloSynth1 opened 8 months ago

SoloSynth1 commented 8 months ago

Submitting Author: Orix Au Yeung (@SoloSynth1) All current maintainers: (@atabak-alishiri, @Marcony1, @shawnhu444) Package Name: VIsuMorph One-Line Description of Package: A package for image manipulations in Python Repository Link: https://github.com/ubc-mds/visumorph Version submitted: v2.0.0 Editor: @ttimbers Reviewer 1: Ella Hein (@ella-irene) Reviewer 2: Riya Eliza Shaju (@riyaeliza123) Reviewer 3: Dan Zhang (@wqxxzd) Reviewer 4: Charles Xu (@charlesxch)

Archive: TBD JOSS DOI: TBD Version accepted: TBD Date accepted (month/day/year): TBD


Code of Conduct & Commitment to Maintain Package

Description

Scope

Domain Specific & Community Partnerships

- [ ] Geospatial
- [ ] Education
- [ ] Pangeo

Community Partnerships

If your package is associated with an existing community please check below:

[^1]: Please fill out a pre-submission inquiry before submitting a data visualization package.

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][JossSubmissionRequirements]. 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][JossSubmissionRequirements]: "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][JossPaperRequirements] 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: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.*

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.

Confirm each of the following by checking the box.

Please fill out our survey

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

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

wqxxzd commented 8 months 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 file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also 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: 1 hour

Review Comments

The package overall is amazing and I have a lot of fun learning it. I really enjoyed trying it out and can see myself using it more. Just a couple of quick suggestions:

riyaeliza123 commented 8 months 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 file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also 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: 30 minutes


Review Comments:

I think this package makes image transformations more convenient. I really like the motivation behind the project. I suggest that you add some more description in "Run the test" section of the READ.ME

ella-irene commented 8 months 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 file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also 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: 1


Review Comments

Overall, really great project! This is very similar to the package my group built, so it was really cool to see which functions you decided to implement, and how you wrote them. I think the structure and contents of this package are very good, and the package is well documented. Furthermore, the motivation is compelling and straight-forward. I've listed below just a few minor notes I had along the way:

charlesxch commented 8 months 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 file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also 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: 1.5 hour


Review Comments

Pros:

Cons:

SoloSynth1 commented 8 months ago

@ella-irene Thank you for your review.

Regarding the error message you encountered, it is most likely that you did not run the pytest command under the project root folder. I was able to replicate the same error message in my environment when I run the pytest command under /Users/<username>/Library/Accounts:

(ubc-mds) ╭─solosynth1@SoloSynth1s-MacBook-Air ~/Library/Accounts
╰─$ pytest                                                                                          1 ↵
========================================= test session starts ==========================================
platform darwin -- Python 3.11.6, pytest-7.4.3, pluggy-1.3.0
rootdir: /Users/solosynth1/Library/Accounts
plugins: anyio-4.0.0
collected 0 items / 1 error

================================================ ERRORS ================================================
____________________________________ ERROR collecting test session _____________________________________
../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/runner.py:341: in from_call
    result: Optional[TResult] = func()
../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/runner.py:372: in <lambda>
    call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/main.py:729: in collect
    for direntry in visit(argpath, self._recurse):
../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/pathlib.py:722: in visit
    entries = scandir(path)
../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/pathlib.py:698: in scandir
    with os.scandir(path) as s:
E   PermissionError: [Errno 1] Operation not permitted: '/Users/solosynth1/Library/Accounts'
=========================================== warnings summary ===========================================
../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/cacheprovider.py:451
  /Users/solosynth1/anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/cacheprovider.py:451: PytestCacheWarning: could not create cache path /Users/solosynth1/Library/Accounts/.pytest_cache/v/cache/nodeids: [Errno 1] Operation not permitted: '/Users/solosynth1/Library/Accounts/.pytest_cache'
    config.cache.set("cache/nodeids", sorted(self.cached_nodeids))

../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/cacheprovider.py:405
  /Users/solosynth1/anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/cacheprovider.py:405: PytestCacheWarning: could not create cache path /Users/solosynth1/Library/Accounts/.pytest_cache/v/cache/lastfailed: [Errno 1] Operation not permitted: '/Users/solosynth1/Library/Accounts/.pytest_cache'
    config.cache.set("cache/lastfailed", self.lastfailed)

../../anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/stepwise.py:56
  /Users/solosynth1/anaconda3/envs/ubc-mds/lib/python3.11/site-packages/_pytest/stepwise.py:56: PytestCacheWarning: could not create cache path /Users/solosynth1/Library/Accounts/.pytest_cache/v/cache/stepwise: [Errno 1] Operation not permitted: '/Users/solosynth1/Library/Accounts/.pytest_cache'
    session.config.cache.set(STEPWISE_CACHE_DIR, [])

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================= short test summary info ========================================
ERROR  - PermissionError: [Errno 1] Operation not permitted: '/Users/solosynth1/Library/Accounts'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
===================================== 3 warnings, 1 error in 0.09s =====================================
╰─$
SoloSynth1 commented 8 months ago

@charlesxch Thank you for your review. Adding links to our GitHub profiles in the README.md sounds like a good idea and I think we will address it in the upcoming release.

Regarding your other comments:

I recommend consolidating the vignettes for various methods into a single document. It can be cumbersome to navigate to different pages to access the examples.

Our team made the deliberate decision to create separate vignettes for each function. It is to avoid unnecessary confusion in explaining the functionalities of each function, as they are all responsible for different operations in image manipulation.

I suggest that you can incorporate the flip, rotate, scale and change_hue methods into the Image class as class methods.

I wholeheartedly agree your suggestion on this. To be honest OOP was our approach initially as these functions work on the same object and naturally fit well with this paradigm.

Unfortunately, the milestone requirements explicitly specify we need to design and implement "user-facing functions".

I do not want to be pedantic as it sounds, but based on this we decided to implement functions instead of class methods out of concerns about implementing class methods might not satisfy the grading requirements.

That being said, I think we will put simple wrappers into the class object in order to address this in the upcoming release.

SoloSynth1 commented 8 months ago

@wqxxzd @riyaeliza123 Thank you for your reviews! The feedbacks have been incorporated in the latest release.