UBC-MDS / software-review-2024

0 stars 0 forks source link

Group 3: financial_analyzer - Peer Review Page #17

Open alanpow opened 8 months ago

alanpow commented 8 months ago

Submitting Author: Alan, Chris, Nicole, Rafe All current maintainers: @alanpow, @chrisgqy, @Nicole-Tu97, @rafecchang Package Name: financial_analyzer One-Line Description of Package: financial_analyzer is a package that helps users solve everyday finance and accounting problems that may arise when running or analyzing a business or service. Repository Link: https://github.com/UBC-MDS/financial_analyzer Version submitted: version 2.0.0 Editor: @ttimbers Reviewer 1: Shawn Hu Reviewer 2: Merete Lutz Reviewer 3: Hancheng Qin Reviewer 4: Paolo De Lagrave-Codina 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 - [X] 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. - [X] 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.

meretelutz 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 hours


Review Comments

  1. I had an error when I was trying to set up the package environment when running the code you included in the README. Running conda create -n --financial_analyzer python=3.9 -y gave me the error conda create: error: argument -n/--name: expected one argument. There wasn't an environment .yml file in the project, but I used my base environment and things were working.
  2. This is a very small issue, but I don't think you should include the function parameters when listing the package's functions in the README. It makes the list very busy to look at and harder to parse through. I also think the code underneath the Usage sub-header is a little redundant, and you could include a link to the documentation or vignette here instead.
  3. Some of the latex in your documentation is not rendering properly. Under "Return on Investment Calculation" the first sentence reads Now we will calcuate the percentage return on investment (ROI,) which is $\frac{\text{current value}-\text{initial investment}}{\text{initial investment}}\times 100$.. There are also a couple small spell check/grammar errors in the documentation. Copy and pasting the text into a word processor and editing would be beneficial.
  4. The CHANGELOG has not been updated for each release of the package. There should be three separate releases (one for each milestone) with description of what has changed underneath.
  5. There is an empty financial_analyzer.py file in the src folder, and an empty test_financial_analyzer.py file in the tests folder. I would suggest deleting these unnecessary files.
  6. I think you have a small error in your README under the License: Alan PowPowichrowski

Overall good job guys! Just some small edits and the issue with the installation instructions, but otherwise good work!

hchqin 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: 1hr


Review Comments

Delightful reading your package and refreshing microeconomics!

  1. Example usage: Under the roi section, some Latex is not rendering correctly.
  2. Example usage: Currently, the four functions share three similar example backgrounds and each has a complete introduction. It might be helpful to reduce some of the redundant information to make the example usage more concise.
  3. pyproject.toml: Package version in the pyproject toml file has not been updated and is still 0.1.0
paolocodina 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:


Review Comments

The document is overall good, well-written, and seems useful.

  1. Including the command 'pip install financial_analyser' before the setup instructions is confusing, as this step should be the final one in the setup process, but it appears at the beginning.
  2. Both the 'src/financial_analyser' and 'dist/financial_analyser' directories contain the 'init.py' and 'function.py' scripts. Having only one copy of these functions would be preferable to avoid repetition.
  3. While it's a minor issue, 'examples.ipynb' could be improved by featuring multiple examples and graphs, rather than just one.
  4. In the unit tests for 'units_for_target_profit', it would have been beneficial to include more extreme cases, such as scenarios with negative profit or non-numeric input values.
shawnhu444 commented 8 months ago

Package Review

Shawnhu444

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

  1. The package is generally good, but it could be more comprehensive and inclusive.
  2. While the package excels in providing comprehensive documentation, including installation instructions and function documentation, it falls short in offering practical examples for all user-facing functions. The inclusion of vignettes and more extensive examples would significantly improve the user experience, aiding in understanding and application of the package's functionality.