nglm / pycvi

Internal Cluster Validity Indices in python, compatible with time-series data
MIT License
4 stars 0 forks source link

JOSS review: basic_example non-functional #4

Closed robcaulk closed 3 months ago

robcaulk commented 5 months ago

Hello,

I am one of the reviewers on the JOSS review for this repository https://github.com/openjournals/joss-reviews/issues/6841.

Following the pip installation instructions located in the docs https://pycvi.readthedocs.io/en/latest/index.html, the examples are not runnable. I copy pasted the example (https://pycvi.readthedocs.io/en/latest/examples/basic_usage.html) into a script, but could see that it would not work. It doesn't work because you have a raw import on the import ..utils plot_true_selected function, which is not part of your pypi package since it is not located within the pyci folder.

In any case, ok, forget the pypi install method, I tried to clone the repo and test, and still ran into some issues:

python3 basic_usage.py 
Traceback (most recent call last):
  File "/home/rcaulk/projects/JOSS_reviews/pycvi/examples/basic_usage/basic_usage.py", line 13, in <module>
    from ..utils import plot_true_selected
ImportError: attempted relative import with no known parent package

The current (wrong) solution would be to copy paste utils.py into the basic_example directory. But generally speaking, if this utils.py is integral for users to learn your code, you should include it in your package so that it can be imported as necessary.

Then we run into the next issue, which is that matplotlib is not installed.

Is matplotlib an "extra" that is not a dependency for this library? If it is not required, but integral for users to learn your code, it would be preferable to at least add this as a doc string to the example scripts. But more likely you would add this to your pypi package pip install pycvi-lib[plot]

After pip install matplotlib, I run into:

python3 basic_usage.py 
Traceback (most recent call last):
  File "/home/rcaulk/projects/JOSS_reviews/pycvi/examples/basic_usage/basic_usage.py", line 13, in <module>
    from utils import plot_true_selected
  File "/home/rcaulk/projects/JOSS_reviews/pycvi/examples/basic_usage/utils.py", line 3, in <module>
    from matplotlib.cm import get_cmap
ImportError: cannot import name 'get_cmap' from 'matplotlib.cm' (/home/rcaulk/.venv/lib/python3.11/site-packages/matplotlib/cm.py)

My matplotlib version seems to fit into your pyproject.toml requirements

pip show matplotlib
Name: matplotlib
Version: 3.9.0
Summary: Python plotting package
Home-page: https://matplotlib.org

And the python version version that I am using is 3.11, which also fits into your pyproject.toml specification.

nglm commented 5 months ago

First all of thank you very much @robcaulk for your great observations! Sometimes when you're too deep into your projects, you don't see the really obvious things anymore!

I hope the updates detailed below have fixed all the mentioned issues, but please let me know if there are still some problems!

Some explanations/context of the issues

Summary of what has been done to fix it