openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
694 stars 36 forks source link

[REVIEW]: pyEQL: A Python interface for water chemistry #6295

Closed editorialbot closed 3 months ago

editorialbot commented 5 months ago

Submitting author: !--author-handle-->@rkingsbury<!--end-author-handle-- (Ryan Kingsbury) Repository: https://github.com/rkingsbury/pyEQL Branch with paper.md (empty if default branch): joss Version: v1.0.0 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @orionarcher, @JacksonBurns, @yuxuanzhuang Archive: 10.5281/zenodo.8332915

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/bdd9e247ea9736a0fdbbd5fe12bef7a6"><img src="https://joss.theoj.org/papers/bdd9e247ea9736a0fdbbd5fe12bef7a6/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/bdd9e247ea9736a0fdbbd5fe12bef7a6/status.svg)](https://joss.theoj.org/papers/bdd9e247ea9736a0fdbbd5fe12bef7a6)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@orionarcher & @JacksonBurns & @yuxuanzhuang, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @lucydot know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

πŸ“ Checklist for @JacksonBurns

πŸ“ Checklist for @yuxuanzhuang

πŸ“ Checklist for @orionarcher

editorialbot commented 5 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 5 months ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.20 s (335.0 files/s, 237153.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JSON                             1              0              0          33315
Python                          28           1810           3736           3951
Markdown                        23            541              0           1907
SVG                              1              1              1            738
YAML                             9             37             30            304
TeX                              1              0              0            189
INI                              1              9              0             82
TOML                             1              7              2             80
Jupyter Notebook                 2              0           1312             54
make                             1              6              8             15
-------------------------------------------------------------------------------
SUM:                            68           2411           5089          40635
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 5 months ago

Wordcount for paper.md is 1316

editorialbot commented 5 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

editorialbot commented 5 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s10953-019-00871-5 is OK
- 10.1016/j.desal.2013.03.015 is OK
- 10.1039/C7NJ03597G is OK
- 10.1016/j.cageo.2011.02.005 is OK
- 10.1016/s0378-3812(02)00178-4 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1063/1.4812323 is OK
- 10.1021/je2009329 is OK
- 10.1016/S0016-7037(97)81133-7 is OK

MISSING DOIs

- None

INVALID DOIs

- None
rkingsbury commented 5 months ago

@editorialbot commands

editorialbot commented 5 months ago

Hello @rkingsbury, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set joss-paper as branch

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers
rkingsbury commented 5 months ago

@lucydot and reviewers, thank you for agreeing to review! Because some time has passed since my original submission, if possible please review the latest version (v0.11.1) rather than the version listed at the top of the thread.

JacksonBurns commented 5 months ago

@rkingsbury no problem, I'll do that version!

@editorialbot generate my checklist doesn't work like this, see https://github.com/openjournals/joss-reviews/issues/6295#issuecomment-1915376354

danielskatz commented 5 months ago

FYI, editorialbot commands need to be the first thing in a new comment

JacksonBurns commented 5 months ago

Review checklist for @JacksonBurns

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

yuxuanzhuang commented 5 months ago

Review checklist for @yuxuanzhuang

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

lucydot commented 5 months ago

@editorialbot set v0.11.1 as version

editorialbot commented 5 months ago

Done! version is now v0.11.1

lucydot commented 4 months ago

Hi @rkingsbury @JacksonBurns @orionarcher - how are your reviews going?

rkingsbury commented 4 months ago

@lucydot I think you mean to tag @yuxuanzhuang . (I am the author!)

JacksonBurns commented 4 months ago

Thanks for the ping @lucydot, review is ongoing on my end. I will be completed within the allotted time.

orionarcher commented 4 months ago

Review checklist for @orionarcher

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

orionarcher commented 4 months ago

Awesome work from @rkingsbury. I encountered no issues installing the package or following along with the examples. This work already appears to be widely used (49 stars) and the buildout of documentation and functionality is sure to make it more useful. I feel this work meets the standards of JOSS and should be accepted.

JacksonBurns commented 4 months ago

Installation and testing was seamless. I was able to follow the examples without issues. I tried entering some non-physical solutions and some ions not in the database and was met with reasonable warnings, which I appreciate.

I will add that the review of 'competing' packages was especially thorough! Well done. Paper overall is concise and convincing, and the software seems outstanding in general.

There are a few small errors I stumbled across that should be fixed, but after that this package looks ready!

List of some small things:

One larger idea, which is out of scope of this review but I thought I might share in case it is helpful (feel free to ignore):

I am sure you are aware of this because of your very thorough requirements directory, but the dependencies are pretty expansive (pip install pyEQL in a bare python 3.9 conda environment installed 94 packages for me). I generated this dependency graph:

tree ``` (pyeql_39) jackson@jackson-Precision-7540:~$ pipdeptree pip==24.0 pipdeptree==2.15.1 pyEQL==0.12.2 β”œβ”€β”€ iapws [required: Any, installed: 1.5.3] β”‚ └── scipy [required: >=1.2, installed: 1.12.0] β”‚ └── numpy [required: >=1.22.4,<1.29.0, installed: 1.26.4] β”œβ”€β”€ maggma [required: >=0.57.5, installed: 0.63.3] β”‚ β”œβ”€β”€ aioitertools [required: >=0.5.1, installed: 0.11.0] β”‚ β”‚ └── typing-extensions [required: >=4.0, installed: 4.10.0] β”‚ β”œβ”€β”€ boto3 [required: >=1.20.41, installed: 1.34.49] β”‚ β”‚ β”œβ”€β”€ botocore [required: >=1.34.49,<1.35.0, installed: 1.34.49] β”‚ β”‚ β”‚ β”œβ”€β”€ jmespath [required: >=0.7.1,<2.0.0, installed: 1.0.1] β”‚ β”‚ β”‚ β”œβ”€β”€ python-dateutil [required: >=2.1,<3.0.0, installed: 2.8.2] β”‚ β”‚ β”‚ β”‚ └── six [required: >=1.5, installed: 1.16.0] β”‚ β”‚ β”‚ └── urllib3 [required: >=1.25.4,<1.27, installed: 1.26.18] β”‚ β”‚ β”œβ”€β”€ jmespath [required: >=0.7.1,<2.0.0, installed: 1.0.1] β”‚ β”‚ └── s3transfer [required: >=0.10.0,<0.11.0, installed: 0.10.0] β”‚ β”‚ └── botocore [required: >=1.33.2,<2.0a.0, installed: 1.34.49] β”‚ β”‚ β”œβ”€β”€ jmespath [required: >=0.7.1,<2.0.0, installed: 1.0.1] β”‚ β”‚ β”œβ”€β”€ python-dateutil [required: >=2.1,<3.0.0, installed: 2.8.2] β”‚ β”‚ β”‚ └── six [required: >=1.5, installed: 1.16.0] β”‚ β”‚ └── urllib3 [required: >=1.25.4,<1.27, installed: 1.26.18] β”‚ β”œβ”€β”€ dnspython [required: >=1.16.0, installed: 2.6.1] β”‚ β”œβ”€β”€ fastapi [required: >=0.42.0, installed: 0.110.0] β”‚ β”‚ β”œβ”€β”€ pydantic [required: >=1.7.4,<3.0.0,!=2.1.0,!=2.0.1,!=2.0.0,!=1.8.1,!=1.8, installed: 2.6.2] β”‚ β”‚ β”‚ β”œβ”€β”€ annotated-types [required: >=0.4.0, installed: 0.6.0] β”‚ β”‚ β”‚ β”œβ”€β”€ pydantic-core [required: ==2.16.3, installed: 2.16.3] β”‚ β”‚ β”‚ β”‚ └── typing-extensions [required: >=4.6.0,!=4.7.0, installed: 4.10.0] β”‚ β”‚ β”‚ └── typing-extensions [required: >=4.6.1, installed: 4.10.0] β”‚ β”‚ β”œβ”€β”€ starlette [required: >=0.36.3,<0.37.0, installed: 0.36.3] β”‚ β”‚ β”‚ β”œβ”€β”€ anyio [required: >=3.4.0,<5, installed: 4.3.0] β”‚ β”‚ β”‚ β”‚ β”œβ”€β”€ exceptiongroup [required: >=1.0.2, installed: 1.2.0] β”‚ β”‚ β”‚ β”‚ β”œβ”€β”€ idna [required: >=2.8, installed: 3.6] β”‚ β”‚ β”‚ β”‚ β”œβ”€β”€ sniffio [required: >=1.1, installed: 1.3.1] β”‚ β”‚ β”‚ β”‚ └── typing-extensions [required: >=4.1, installed: 4.10.0] β”‚ β”‚ β”‚ └── typing-extensions [required: >=3.10.0, installed: 4.10.0] β”‚ β”‚ └── typing-extensions [required: >=4.8.0, installed: 4.10.0] β”‚ β”œβ”€β”€ jsonschema [required: >=3.1.1, installed: 4.21.1] β”‚ β”‚ β”œβ”€β”€ attrs [required: >=22.2.0, installed: 23.2.0] β”‚ β”‚ β”œβ”€β”€ jsonschema-specifications [required: >=2023.03.6, installed: 2023.12.1] β”‚ β”‚ β”‚ └── referencing [required: >=0.31.0, installed: 0.33.0] β”‚ β”‚ β”‚ β”œβ”€β”€ attrs [required: >=22.2.0, installed: 23.2.0] β”‚ β”‚ β”‚ └── rpds-py [required: >=0.7.0, installed: 0.18.0] β”‚ β”‚ β”œβ”€β”€ referencing [required: >=0.28.4, installed: 0.33.0] β”‚ β”‚ β”‚ β”œβ”€β”€ attrs [required: >=22.2.0, installed: 23.2.0] β”‚ β”‚ β”‚ └── rpds-py [required: >=0.7.0, installed: 0.18.0] β”‚ β”‚ └── rpds-py [required: >=0.7.1, installed: 0.18.0] β”‚ β”œβ”€β”€ mongogrant [required: >=0.3.1, installed: 0.3.3] β”‚ β”‚ β”œβ”€β”€ click [required: Any, installed: 8.1.7] β”‚ β”‚ β”œβ”€β”€ flask [required: >=1.0, installed: 3.0.2] β”‚ β”‚ β”‚ β”œβ”€β”€ blinker [required: >=1.6.2, installed: 1.7.0] β”‚ β”‚ β”‚ β”œβ”€β”€ click [required: >=8.1.3, installed: 8.1.7] β”‚ β”‚ β”‚ β”œβ”€β”€ importlib-metadata [required: >=3.6.0, installed: 7.0.1] β”‚ β”‚ β”‚ β”‚ └── zipp [required: >=0.5, installed: 3.17.0] β”‚ β”‚ β”‚ β”œβ”€β”€ itsdangerous [required: >=2.1.2, installed: 2.1.2] β”‚ β”‚ β”‚ β”œβ”€β”€ Jinja2 [required: >=3.1.2, installed: 3.1.3] β”‚ β”‚ β”‚ β”‚ └── MarkupSafe [required: >=2.0, installed: 2.1.5] β”‚ β”‚ β”‚ └── werkzeug [required: >=3.0.0, installed: 3.0.1] β”‚ β”‚ β”‚ └── MarkupSafe [required: >=2.1.1, installed: 2.1.5] β”‚ β”‚ β”œβ”€β”€ pymongo [required: >=3.8, installed: 4.6.2] β”‚ β”‚ β”‚ └── dnspython [required: >=1.16.0,<3.0.0, installed: 2.6.1] β”‚ β”‚ └── requests [required: Any, installed: 2.31.0] β”‚ β”‚ β”œβ”€β”€ certifi [required: >=2017.4.17, installed: 2024.2.2] β”‚ β”‚ β”œβ”€β”€ charset-normalizer [required: >=2,<4, installed: 3.3.2] β”‚ β”‚ β”œβ”€β”€ idna [required: >=2.5,<4, installed: 3.6] β”‚ β”‚ └── urllib3 [required: >=1.21.1,<3, installed: 1.26.18] β”‚ β”œβ”€β”€ mongomock [required: >=3.10.0, installed: 4.1.2] β”‚ β”‚ β”œβ”€β”€ packaging [required: Any, installed: 23.2] β”‚ β”‚ └── sentinels [required: Any, installed: 1.0.0] β”‚ β”œβ”€β”€ monty [required: >=2023.9.25, installed: 2024.2.26] β”‚ β”œβ”€β”€ msgpack [required: >=0.5.6, installed: 1.0.7] β”‚ β”œβ”€β”€ numpy [required: >=1.17.3, installed: 1.26.4] β”‚ β”œβ”€β”€ orjson [required: >=3.9.0, installed: 3.9.15] β”‚ β”œβ”€β”€ pydantic [required: >=2.0, installed: 2.6.2] β”‚ β”‚ β”œβ”€β”€ annotated-types [required: >=0.4.0, installed: 0.6.0] β”‚ β”‚ β”œβ”€β”€ pydantic-core [required: ==2.16.3, installed: 2.16.3] β”‚ β”‚ β”‚ └── typing-extensions [required: >=4.6.0,!=4.7.0, installed: 4.10.0] β”‚ β”‚ └── typing-extensions [required: >=4.6.1, installed: 4.10.0] β”‚ β”œβ”€β”€ pydantic-settings [required: >=2.0.3, installed: 2.2.1] β”‚ β”‚ β”œβ”€β”€ pydantic [required: >=2.3.0, installed: 2.6.2] β”‚ β”‚ β”‚ β”œβ”€β”€ annotated-types [required: >=0.4.0, installed: 0.6.0] β”‚ β”‚ β”‚ β”œβ”€β”€ pydantic-core [required: ==2.16.3, installed: 2.16.3] β”‚ β”‚ β”‚ β”‚ └── typing-extensions [required: >=4.6.0,!=4.7.0, installed: 4.10.0] β”‚ β”‚ β”‚ └── typing-extensions [required: >=4.6.1, installed: 4.10.0] β”‚ β”‚ └── python-dotenv [required: >=0.21.0, installed: 1.0.1] β”‚ β”œβ”€β”€ pydash [required: >=4.1.0, installed: 7.0.7] β”‚ β”‚ └── typing-extensions [required: >=3.10,!=4.6.0, installed: 4.10.0] β”‚ β”œβ”€β”€ pymongo [required: >=4.2.0, installed: 4.6.2] β”‚ β”‚ └── dnspython [required: >=1.16.0,<3.0.0, installed: 2.6.1] β”‚ β”œβ”€β”€ python-dateutil [required: >=2.8.2, installed: 2.8.2] β”‚ β”‚ └── six [required: >=1.5, installed: 1.16.0] β”‚ β”œβ”€β”€ pyzmq [required: >=24.0.1, installed: 25.1.2] β”‚ β”œβ”€β”€ ruamel.yaml [required: <0.18, installed: 0.17.40] β”‚ β”‚ └── ruamel.yaml.clib [required: >=0.2.7, installed: 0.2.8] β”‚ β”œβ”€β”€ setuptools [required: Any, installed: 69.1.1] β”‚ β”œβ”€β”€ sshtunnel [required: >=0.1.5, installed: 0.4.0] β”‚ β”‚ └── paramiko [required: >=2.7.2, installed: 3.4.0] β”‚ β”‚ β”œβ”€β”€ bcrypt [required: >=3.2, installed: 4.1.2] β”‚ β”‚ β”œβ”€β”€ cryptography [required: >=3.3, installed: 42.0.5] β”‚ β”‚ β”‚ └── cffi [required: >=1.12, installed: 1.16.0] β”‚ β”‚ β”‚ └── pycparser [required: Any, installed: 2.21] β”‚ β”‚ └── PyNaCl [required: >=1.5, installed: 1.5.0] β”‚ β”‚ └── cffi [required: >=1.4.1, installed: 1.16.0] β”‚ β”‚ └── pycparser [required: Any, installed: 2.21] β”‚ β”œβ”€β”€ tqdm [required: >=4.19.6, installed: 4.66.2] β”‚ └── uvicorn [required: >=0.18.3, installed: 0.27.1] β”‚ β”œβ”€β”€ click [required: >=7.0, installed: 8.1.7] β”‚ β”œβ”€β”€ h11 [required: >=0.8, installed: 0.14.0] β”‚ └── typing-extensions [required: >=4.0, installed: 4.10.0] β”œβ”€β”€ monty [required: Any, installed: 2024.2.26] β”œβ”€β”€ numpy [required: Any, installed: 1.26.4] β”œβ”€β”€ phreeqpython [required: Any, installed: 1.5.0] β”‚ β”œβ”€β”€ periodictable [required: Any, installed: 1.6.1] β”‚ β”‚ β”œβ”€β”€ numpy [required: Any, installed: 1.26.4] β”‚ β”‚ └── pyparsing [required: Any, installed: 3.1.1] β”‚ └── scipy [required: Any, installed: 1.12.0] β”‚ └── numpy [required: >=1.22.4,<1.29.0, installed: 1.26.4] β”œβ”€β”€ Pint [required: >=0.19, installed: 0.23] β”‚ └── typing-extensions [required: Any, installed: 4.10.0] β”œβ”€β”€ pymatgen [required: >=2023.10.11, installed: 2024.2.23] β”‚ β”œβ”€β”€ joblib [required: Any, installed: 1.3.2] β”‚ β”œβ”€β”€ matplotlib [required: >=1.5, installed: 3.8.3] β”‚ β”‚ β”œβ”€β”€ contourpy [required: >=1.0.1, installed: 1.2.0] β”‚ β”‚ β”‚ └── numpy [required: >=1.20,<2.0, installed: 1.26.4] β”‚ β”‚ β”œβ”€β”€ cycler [required: >=0.10, installed: 0.12.1] β”‚ β”‚ β”œβ”€β”€ fonttools [required: >=4.22.0, installed: 4.49.0] β”‚ β”‚ β”œβ”€β”€ importlib-resources [required: >=3.2.0, installed: 6.1.2] β”‚ β”‚ β”‚ └── zipp [required: >=3.1.0, installed: 3.17.0] β”‚ β”‚ β”œβ”€β”€ kiwisolver [required: >=1.3.1, installed: 1.4.5] β”‚ β”‚ β”œβ”€β”€ numpy [required: >=1.21,<2, installed: 1.26.4] β”‚ β”‚ β”œβ”€β”€ packaging [required: >=20.0, installed: 23.2] β”‚ β”‚ β”œβ”€β”€ pillow [required: >=8, installed: 10.2.0] β”‚ β”‚ β”œβ”€β”€ pyparsing [required: >=2.3.1, installed: 3.1.1] β”‚ β”‚ └── python-dateutil [required: >=2.7, installed: 2.8.2] β”‚ β”‚ └── six [required: >=1.5, installed: 1.16.0] β”‚ β”œβ”€β”€ monty [required: >=2024.2.2, installed: 2024.2.26] β”‚ β”œβ”€β”€ networkx [required: >=2.2, installed: 3.2.1] β”‚ β”œβ”€β”€ numpy [required: >=1.25.0, installed: 1.26.4] β”‚ β”œβ”€β”€ palettable [required: >=3.1.1, installed: 3.3.3] β”‚ β”œβ”€β”€ pandas [required: Any, installed: 2.2.1] β”‚ β”‚ β”œβ”€β”€ numpy [required: >=1.22.4,<2, installed: 1.26.4] β”‚ β”‚ β”œβ”€β”€ python-dateutil [required: >=2.8.2, installed: 2.8.2] β”‚ β”‚ β”‚ └── six [required: >=1.5, installed: 1.16.0] β”‚ β”‚ β”œβ”€β”€ pytz [required: >=2020.1, installed: 2024.1] β”‚ β”‚ └── tzdata [required: >=2022.7, installed: 2024.1] β”‚ β”œβ”€β”€ plotly [required: >=4.5.0, installed: 5.19.0] β”‚ β”‚ β”œβ”€β”€ packaging [required: Any, installed: 23.2] β”‚ β”‚ └── tenacity [required: >=6.2.0, installed: 8.2.3] β”‚ β”œβ”€β”€ pybtex [required: Any, installed: 0.24.0] β”‚ β”‚ β”œβ”€β”€ latexcodec [required: >=1.0.4, installed: 2.0.1] β”‚ β”‚ β”‚ └── six [required: >=1.4.1, installed: 1.16.0] β”‚ β”‚ β”œβ”€β”€ PyYAML [required: >=3.01, installed: 6.0.1] β”‚ β”‚ └── six [required: Any, installed: 1.16.0] β”‚ β”œβ”€β”€ requests [required: Any, installed: 2.31.0] β”‚ β”‚ β”œβ”€β”€ certifi [required: >=2017.4.17, installed: 2024.2.2] β”‚ β”‚ β”œβ”€β”€ charset-normalizer [required: >=2,<4, installed: 3.3.2] β”‚ β”‚ β”œβ”€β”€ idna [required: >=2.5,<4, installed: 3.6] β”‚ β”‚ └── urllib3 [required: >=1.21.1,<3, installed: 1.26.18] β”‚ β”œβ”€β”€ ruamel.yaml [required: >=0.17.0, installed: 0.17.40] β”‚ β”‚ └── ruamel.yaml.clib [required: >=0.2.7, installed: 0.2.8] β”‚ β”œβ”€β”€ scipy [required: >=1.5.0, installed: 1.12.0] β”‚ β”‚ └── numpy [required: >=1.22.4,<1.29.0, installed: 1.26.4] β”‚ β”œβ”€β”€ spglib [required: >=2.0.2, installed: 2.3.1] β”‚ β”‚ └── numpy [required: Any, installed: 1.26.4] β”‚ β”œβ”€β”€ sympy [required: Any, installed: 1.12] β”‚ β”‚ └── mpmath [required: >=0.19, installed: 1.3.0] β”‚ β”œβ”€β”€ tabulate [required: Any, installed: 0.9.0] β”‚ β”œβ”€β”€ tqdm [required: Any, installed: 4.66.2] β”‚ └── uncertainties [required: >=3.1.4, installed: 3.1.7] β”‚ └── future [required: Any, installed: 1.0.0] └── scipy [required: Any, installed: 1.12.0] └── numpy [required: >=1.22.4,<1.29.0, installed: 1.26.4] wheel==0.42.0 ```

and it seems that most of the bulk is coming from maggma and pymatgen. If most of your end users are within the materials project world I don't see this being a problem, but it may impact reuse potential for those trying to extend pyEQL into other domains. If possible, could these be removed or made optional (to maintain optional perfect compatibility with materials project)? A quick search of the pyEQL repo shows that pymatgen (as also described in the README) is only used to standardize some input data - could this be achieved with another (smaller) package? Similarly for maggma, search (and README) show it used only for serializing and de-serealizing - could you wrap pymongo yourself and remove maggma?

rkingsbury commented 4 months ago

Installation and testing was seamless. I was able to follow the examples without issues. I tried entering some non-physical solutions and some ions not in the database and was met with reasonable warnings, which I appreciate.

I will add that the review of 'competing' packages was especially thorough! Well done. Paper overall is concise and convincing, and the software seems outstanding in general.

Thank you very much @JacksonBurns ! I appreciate your thorough testing and identifying the documentation issues.

One larger idea, which is out of scope of this review but I thought I might share in case it is helpful (feel free to ignore):

I am sure you are aware of this because of your very thorough requirements directory, but the dependencies are pretty expansive (pip install pyEQL in a bare python 3.9 conda environment installed 94 packages for me). I generated this dependency graph: tree

Great suggestions. I have thought about this in the abstract but I did not realize the extent of the dependency bloat until seeing the tree you generated (aside: I was unaware of pipdeptree, so thanks for that!). I am a maintainer of maggma and I think we should explore more optional dependency groups to manage that, because many of those are not needed for basic mongo-style querying. Or, as you suggest, for the specific ways we use maggma in pyEQL, we might be able to get away with a custom solution.

You are correct that the main purpose of having pymatgen in here is so that formulas can be standardized in a way that pymatgen will understand, and also to leverage pymatgen for chemical informatics such as molecular weights, oxidation states, etc. based on those formulas. There are alternatives, but I know that efforts are underway to make pymatgen itself leaner so I think it's likely I will stick with it in pyEQL for the near future.

lucydot commented 4 months ago

It looks like this is progressing very nicely - thanks for your thorough reviews @orionarcher and @JacksonBurns ✨

@yuxuanzhuang - how is your review going? I can see most items are checked, there are a couple of outstanding points in documentation.

yuxuanzhuang commented 4 months ago

I love the features of this package and even plan to add it to my research toolbox. I've started a separate issue about documentation issues in the repository, which the author is already addressing. Aside from that, the software is well-documented and the tests are abundant.

I do have an additional comment/question: It appears that the package does not support Mac computers equipped with Apple silicon (although it can be installed without any issue). This seems to be due to the phreeqpython package lacks support for it:

> s1 = Solution()
OSError: dlopen(/xxx/lib/python3.10/site-packages/phreeqpython/./lib/viphreeqc.dylib, 0x0006): tried: '/xxx/lib/python3.10/site-packages/phreeqpython/./lib/viphreeqc.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')), 

Perhaps it's better to document this compatibility issue somewhere?

I would definitely recommend it to be accepted. Great work!!!

rkingsbury commented 4 months ago

I love the features of this package and even plan to add it to my research toolbox. I've started a separate issue about documentation issues in the repository, which the author is already addressing. Aside from that, the software is well-documented and the tests are abundant.

I would definitely recommend it to be accepted. Great work!!!

Thank you very much @yuxuanzhuang ! I'm thrilled to hear that you might find pyEQL useful in your own research.

I do have an additional comment/question: It appears that the package does not support Mac computers equipped with Apple silicon (although it can be installed without any issue). This seems to be due to the phreeqpython package lacks support for it:

> s1 = Solution()
OSError: dlopen(/xxx/lib/python3.10/site-packages/phreeqpython/./lib/viphreeqc.dylib, 0x0006): tried: '/xxx/lib/python3.10/site-packages/phreeqpython/./lib/viphreeqc.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')), 

Perhaps it's better to document this compatibility issue somewhere?

This is an excellent finding that I had not considered before, thank you for reporting.

rkingsbury commented 4 months ago

@lucydot it looks like reviews are complete. What are the next steps?

lucydot commented 4 months ago

Excellent - I can see some of the issues raised as part of the review are not yet addressed, though reading the discussion it seems that these are not acceptance-blockers but suggestions for releases beyond. Great work @rkingsbury and team, and for your reviews @yuxuanzhuang, @orionarcher, @JacksonBurns.

I'm going to generate the post-review checklist - please let me know when each item is complete @rkingsbury.

lucydot commented 4 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

rkingsbury commented 4 months ago

@lucydot all items complete! The version is v0.14.0 and the Zenodo DOI is https://doi.org/10.5281/zenodo.10783921 (for this specific version). As I'm sure you know, Zenodo also provides an "all versions" DOI.

lucydot commented 4 months ago

Thanks @rkingsbury

We usually recommend semantic versioning, where a 0.x.x release would suggest the software is in initial development and unstable. Would you consider moving to a major release? To be clear, it is not an acceptance blocker, rather a recommendation. We understand there may be exceptions for various reasons.

lucydot commented 4 months ago

@editorialbot set 10.5281/zenodo.8332915 as archive

editorialbot commented 4 months ago

Done! archive is now 10.5281/zenodo.8332915

lucydot commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

lucydot commented 4 months ago

@rkingsbury - the title of the Zenodo release needs to exactly match that in the JOSS paper - can you update?

lucydot commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s10953-019-00871-5 is OK
- 10.1016/j.desal.2013.03.015 is OK
- 10.1039/C7NJ03597G is OK
- 10.1016/j.cageo.2011.02.005 is OK
- 10.1016/s0378-3812(02)00178-4 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1063/1.4812323 is OK
- 10.1021/je2009329 is OK
- 10.1016/S0016-7037(97)81133-7 is OK

MISSING DOIs

- No DOI given, and none found for title: Description of Input and Examples for PHREEQC Vers...
- No DOI given, and none found for title: Ionic Conductivity and Diffusion at Infinite Dilut...
- No DOI given, and none found for title: PyEquIon: A Python Package For Automatic Speciatio...
- No DOI given, and none found for title: PyEquIon: A Python Package For Automatic Speciatio...
- No DOI given, and none found for title: Phreeqpython
- No DOI given, and none found for title: Pint: makes units easy
- No DOI given, and none found for title: Maggma: A files-to-API data pipeline for scientifi...
- No DOI given, and none found for title: The Geochemist’s Workbench, Release 17

INVALID DOIs

- None
lucydot commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s10953-019-00871-5 is OK
- 10.1016/j.desal.2013.03.015 is OK
- 10.1039/C7NJ03597G is OK
- 10.1016/j.cageo.2011.02.005 is OK
- 10.1016/s0378-3812(02)00178-4 is OK
- 10.1016/j.commatsci.2012.10.028 is OK
- 10.1063/1.4812323 is OK
- 10.1021/je2009329 is OK
- 10.1016/S0016-7037(97)81133-7 is OK

MISSING DOIs

- No DOI given, and none found for title: Description of Input and Examples for PHREEQC Vers...
- No DOI given, and none found for title: Ionic Conductivity and Diffusion at Infinite Dilut...
- No DOI given, and none found for title: PyEquIon: A Python Package For Automatic Speciatio...
- No DOI given, and none found for title: PyEquIon: A Python Package For Automatic Speciatio...
- No DOI given, and none found for title: Phreeqpython
- No DOI given, and none found for title: Pint: makes units easy
- No DOI given, and none found for title: Maggma: A files-to-API data pipeline for scientifi...
- No DOI given, and none found for title: The Geochemist’s Workbench, Release 17

INVALID DOIs

- None
lucydot commented 4 months ago

One more comment πŸ˜„ ...

Whilst reading through paper @rkingsbury I've spotted some adjustments to make:

Once you make adjustments, please check they are as expected using @editorialbot generate pdf.

lucydot commented 4 months ago

Hello all, a quick message to say I will be out of office from tomorrow, and will check back here on the 18th.

rkingsbury commented 4 months ago

Hello all, a quick message to say I will be out of office from tomorrow, and will check back here on the 18th.

OK, thanks for letting me know @lucydot ! I like your idea about bumping the version to 1.0 (I was thinking about this even before you posted). There are a few small changes I want to make before I do so, but I'll take care of those soon and let you know when ready.

One question - after I make the requested corrections to the manuscript, approximately how long does final acceptance usually take? I ask because I am going to feature pyEQL in a conference talk the week of the 18th, and I'd love to be able to say it has been accepted.

rkingsbury commented 4 months ago

(If helpful, I can try to make changes to the manuscript and Zenodo title today)

rkingsbury commented 4 months ago
rkingsbury commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

rkingsbury commented 3 months ago

@editorialbot generate pdf

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

rkingsbury commented 3 months ago

@editorialbot generate pdf

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

rkingsbury commented 3 months ago

@editorialbot generate pdf