UBC-MDS / software-review-2022

0 stars 0 forks source link

Submission Group 30: chemcalculator (Python) #9

Open Kingslin0810 opened 2 years ago

Kingslin0810 commented 2 years ago

Submitting Authors:

Package Name: chemcalculator One-Line Description of Package: chemcalculator is a python package useful for chemistry for purpose of calculating chemical formular mass in g/mol. Repository Link: chemcalculator Version submitted: 1.0.1 Editor: Florencia D'Andrea (@flor14)

Description

Python package to calculate chemical formula or atomic mass (g/mol), convert moles to grams and grams to moles, and calculate percentage mass for chemical or atom of interest

Other category: scientific software wrappers . The package chemcalculator converts between moles and grams, helping researchers or data scientist to perform this widely used conversions fast and without errors.

Data scientists or researchers working with chemistry data.

Are there other Python packages that accomplish the same thing? If so, how does yours differ?

No. This package of basic chemistry calculations is meant to supplement an existing package, ChemPy, which already handles complex calculations for primarily physical/inorganic/analytical chemistry consisting of, but not limited to, the following:

  • Solver for equilibria (including multiphase systems)
  • Numerical integration routines for chemical kinetics (ODE solver front-end)
  • Integrated rate expressions (and convenience fitting routines)
  • Relations in Physical chemistry
  • Debye-Hückel expressions
  • Arrhenius equation
  • Einstein-Smoluchowski equation
  • Properties, such as : water density as function of temperature, water permittivity as function of temperature and pressure, and water diffusivity as function of temperature
JOSS Checks * [ ] The package has an obvious research application according to JOSS's definition in their 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: "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 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. * [x] Yes I am OK with reviewers submitting requested changes as issues to my repo. Reviewers will then link to the issues in their submitted review. ## Code of conduct * [x] I agree to abide by [pyOpenSci's Code of Conduct](https://www.pyopensci.org/contributing-guide/open-source-software-peer-review/code-of-conduct.html?highlight=code%20conduct) during the review process and in maintaining my package should it be accepted. **P.S.** *Have feedback/comments about our review process? Leave a comment [here](https://github.com/pyOpenSci/governance/issues/8) ## Editor and Review Templates [Editor and review templates can be found here](https://www.pyopensci.org/contributing-guide/appendices/templates.html)
khbunyan commented 2 years ago

Package Review

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:

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:

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:

2

Review Comments

Hi Group 30, well done on your package! It's clean, concise, and is very practical for chemists. Overall, I think your project is really well put together. Some particular highlights are; everything being clearly organized, mostly everything required being included and easy to find, and the written documentation being very well organized so it's simple to follow.

I have a few notes for some areas that I think you could explore for improvement:

  1. In your "Usage" section, there is a small typo in the example for percent_mass. Right now it says the number 0 instead of the letter O for the second input: percent_mass("H2O", "0"). I think you intended to use the letter O here.
  2. I think moles_grams_converter() might actually be performing the reverse of what is intended here. Screen Shot 2022-01-31 at 6 30 46 PM
    • Based on your "read the docs" your function should be converting grams to moles, but it's actually converting moles to grams in the above example.
    • The docstring for this function tells me that the "convert_to" variable is what the function is converting my "mass" value to, ie the final units of the output value. So, if we input 0.05555 (assuming that's grams) and use "moles" as the convert_to input, it should result in 0.0030835 mols, as 0.05555 grams is equivalent to 0.0030835 mols. However the result is 1.00.
    • This would be correct if we were converting from moles not converting to moles, as 0.05555 moles = 1.00 gram.
    • The translation code is working properly, but convert_to should be convert_from.
  3. A few small grammatical errors in the README description:
    • "chemcalculator is a python package useful for chemistry for THE purpose of calculating chemical formular mass in g/mol."
    • "Another property of Avogadro’s number is that the mass of one mole of a substance is equal to that substance’s molecular weight." Avogadro’s number is not defined - is this a synonym for mols? I'm not a chemist so I could be missing something that is common knowledge here.
  4. The badges for codecov and docs look great, but there is no ci-cd badge in the README. Please add so there is confirmation of the workflow's passing without having to go into the actions tab.
  5. I would suggest removing branches that are no longer needed, such as "ci-cd" and "update_README". Keeps the repo cleaner.

In addition to those 5 comments, there are some missed check boxes above that you could review as well, some are not applicable for your project such as the JOSS section. I downloaded your repo and ran the pytests on my machine, and everything passed. Read the docs, and your workflow was reviewed and everything looked good there as well. Again, great work team, I really enjoyed reading through this. Thank you.

jo4356 commented 2 years ago

@khbunyan Thank you for the detailed review! We have fixed the moles_grams_converter() not behaving as desired in https://github.com/UBC-MDS/chemcalculator/commit/f9adc339806ca2dcc514c6120a30dbce9eacdac5 and https://github.com/UBC-MDS/chemcalculator/commit/7546f8befe566824c22cc7e9c7b3d72a2dde3220

thayeylolu commented 2 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

All in all, I believe this is an awesome job. I have very little recommendations, observations and suggestion.

  1. I observed that all the functions and tests are written in one file . I would recommend separating it in the future to avoid merge conflicts and easy expansion of functions in the future.

  2. As @khbunyan mentioned, there are some typos in the Readme.md

  3. on line 91 of this script, I observed that the parameter's description is missing a full-stop (I believe this package is following the numpy docstyle)

  4. In the enforcement section, of this page Here your contact email address appears twice. I suggest you make it appear once. Same for this page Here

  5. I also observed you list just three contributors but your contributors list on your github home page has four.

berkaybulut commented 2 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:

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:

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

Hello group 30, congratulations on your repository. It is a unique repo that is focused on chemists in python space, and it is one of the first that I have come across something like this. I think your repository is well organized and easy to understand. It is well put together and simple to follow.

I have a few comments that you can work on to improve further.

  1. You functions are all in in one file, it can be improved to separate them for future ease of upgradability.
  2. There are some typos in Readme.md as mentioned by @thayeylolu and @khbunyan
  3. In your helper.py file the __check_chemical_format() function is missing a full stop typo in parameter description - according to numpy docstyle.

Great work to everyone - let me know if you have any questions!

jo4356 commented 2 years ago

@thayeylolu Thanks for your review! To address your comment about contributor numbers, we only have 3 contributors. Two of the accounts listed as contributors belongs to the same person.

helingogo commented 2 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:

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:

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


Review Comments

Congratulation on the completion of your project and the 100% code coverage! The repo is well organized and clean. The topic is explained well so that I can easily understand even though I am from business background. Most parts look pretty good. Following are some problems and suggestions:

  1. The result I have for compute_mass("H2O") is 18.013, which is different from what you demonstrate in readme and documentation.

  2. I think it is better to put outcome or result in Usage part both in readme file and in documentation so that the users would know whether they download and use the package correctly.

  3. Also, elements for functions to be filled can be added and explained in the documentation and users would understand what the function does better. For example, users may need and want to know the third element of moles_grams_converter("H2O", 0.05555, **"moles"**) is converting from or converting to.

  4. Dependencies can be added in readme file and documentation file.

  5. You can also separate the test file into several ones like what has been done for functions.

It is a pleasure to look through your repo. Congrats again! Let me know if you have any question.