Santiso-Group / despasito

DESPASITO: Determining Equilibrium State and Parametrization Application for SAFT, Intended for Thermodynamic Output
https://despasito.readthedocs.io
BSD 3-Clause "New" or "Revised" License
14 stars 9 forks source link

[JOSS Review] Inconsistency between documentation and the PyPI source #33

Closed yuzie007 closed 5 days ago

yuzie007 commented 2 weeks ago

(This is a part of my review for JOSS https://github.com/openjournals/joss-reviews/issues/7365. cc: @srmnitc)

Dear @jaclark5 and other authors of DESPASITO,

I am Yuji Ikeda @yuzie007 and reviewing DESPASITO for JOSS. Thank you for writing a wonderful code and making it open-source.

Following the JOSS guideline, I am first checking the consistency with the documentation and the actual code in the PyPI source code. Could you check the points below and, when necessary, update the repo?

I will also (1) test other examples and (2) read the paper submitted to JOSS. The feedback for these will be reported in other issues.

During handling of the above exception, another exception occurred:

Traceback (most recent call last): File "/Users/ikeda/codes/despasito/despasito/examples/saft_gamma_mie/hexane_heptane_activity_coeff/test.py", line 18, in output = thermo.thermo( ^^^^^^^^^^^^^^ File "/Users/ikeda/miniforge3/envs/myenv/lib/python3.11/site-packages/despasito/thermodynamics/init.py", line 63, in thermo raise TypeError("The calculation type, '{}', failed".format(calculation_type)) TypeError: The calculation type, 'vapor_properties', failed

and

Traceback (most recent call last): File "/Users/ikeda/codes/despasito/despasito/examples/saft_gamma_mie/hexane_heptane_activity_coeff/hexane_heptane_test.py", line 8, in despasito.initiate_logger(console=True, verbose=10) File "/Users/ikeda/miniforge3/envs/myenv/lib/python3.11/site-packages/despasito/init.py", line 95, in initiate_logger handler_logfile.close() ^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'close'



- [x] **Documentation** – **Example usage**: In [`startfitting.rst`](https://github.com/Santiso-Group/despasito/blob/0851b58d874175cb11afc82bf2622c1dd474f617/docs/startfitting.rst?plain=1#L17), it is written that the `input_fit.json` for methanol can be found in the `examples` directory. However, I could only found it fro another system in `examples/saft_gamma_mie/butane_solubility`. (I could run `input_fit.json` in this directory.)
JacksonBurns commented 1 week ago

Hi I am the other reviewer. I also noticed the inconsistent listing of support Python versions between the README and the CI, but also in the setup.py file, which allows any modern version:

    python_requires=">=3.6",
jaclark5 commented 1 week ago

@yuzie007 Thank you for your comments and pointing out the inconsistencies. I've addressed them in #39 if you'd like to view them. Notice that the docs fail due to the update in dependencies required that have been included in PR 38.

Although I updated the tested versions of python and the documentation to reflect that, this PR will likely have a few more commits if there are issues with later versions of python. I will comment again when all tests except for readthedocs are clear.

jaclark5 commented 1 week ago

@JacksonBurns I've addressed the inconsistencies between the paper, README, and CI testing. I will keep the setup.py file as is so that installation won't automatically fail with future releases of python, quite possibly without cause.

jaclark5 commented 1 week ago

@yuzie007 all tests have passed for python 3.10, 3.11, and 3.12. python 3.13 is not yet supported by the condo-incubator used in GitHub actions.

yuzie007 commented 6 days ago

Thank you so much for #39 and information. Regarding Python 3.13, if it cannot be tested just due to the GitHub action issue, I would feel it is OK to also include Python 3.13 also in the support, if it can pass all tests, e.g., in a local environment.

After #38 and #39 are merged, I would like to try remaining tests, and then I think I will finish my JOSS review.

jaclark5 commented 5 days ago

Both #38 and #39 have been merged. #40 contains changes from the other reviewer.