Review JOSS vahid-sb

[vahid-sb]

Hi guys
I am testing your library as part of my review for JOSS and I opened this issue to share all the issues I find in the repo.
Firstly, congrats on releasing this library. It seems to be the culmination of a hard and long effort and I am sure it will be useful for many researchers and practitioners.

I cloned your repo on my local computer which runs Ubuntu 22.04. I created a new Anaconda environment with Python=3.9 and installed the library using conda. Then installed the library from the local clone using pip install -e . and executed the scripts in Spyder IDE.

Here are the issues, so far:

1. A minor one, the link to the installation guide in the readme file is broken.
2. while running plot_group_level_with_individual_clusters.py from the tutorials folder, I got an error that pandas is missing (line 66). Although not used in the main library, but does not hurt to add pandas to dependencies.
3. As a suggestion, maybe you can add plt.close('all') to the beginning of the tutorial scripts. After running a few ones, hard to figure out which ones are generated by the most recent script.
4. readme.rst in /tutorials/preprocessing says Below is a gallery of examples. without providing any examples. It is better to remove the file rather than have an incomplete one.
5. I ran all the tests in all subfolders, and I did not face any errors, except missing pytest library which is a minor one. But more importantly, relative imports in /datasets/lemon/lemon.py (lines 15 and 16). Replacing ...utils._checks and ...utils._config with pycrostates.utils._checks and pycrostates.utils._config fixes the issue.
6. All scripts in /metrics also use relative imports which causes an error. Again, using absolute imports fixes the issue.

That's it for now.

[mscheltienne]

Hello, thank you for your feedback and review!

I will fix (1) and (4) as part of #58.

For (2), I'm a bit reluctant to make pandas a hard dependency just for a tutorial. It seems like it's only used for a display as a DataFrame at the end of the tutorial, I think it would be nice if we could just get rid of it and display this part of the tutortial differently. @vferat WDYT?

For (5) and (6), I'm not sure why you had to change the relative imports to absolute imports. As most python package, we use relative imports in pycrostates to classes and function from pycrostates. The 2 examples you cite are not the only use of relative imports in pycrostates. You will find the same use of relative import in other packages, e.g. scikit-learn: https://github.com/scikit-learn/scikit-learn/blob/f0cbdc15f5031f0595ffcc8f822e5318a2335349/sklearn/cross_decomposition/_pls.py#L1-L27
It might be related to an installation step you did. As of MNE-Python 1.1, pycrostates is also distributed in the mne-installers: https://mne.tools/stable/install/index.html. A link to the installers is also available on the pycrostates documentation. This is the easiest way to install it: it will create a conda environment with the entire MNE-ecosystem, with all the additional dependencies setup, and with IDE, Jupyter, pandas, ... and other commonly used software/packages in python. It also has the benefit of being tested on different OS, including M1 macOS.
As part of #58, I am also improving the installation documentation of pycrostates.

[vferat]

Hey @vahid-sb thanks for your time playing with the code and for your review !

For (2), I agree that it is no worth adding pandas only to display some results in only one tutorial. We can either create an installation configuration pycrostates[tutorials] which will include pandas or simply not using it in the tutorial.

For (5) and (6), I tried to replicate the issue by running pytest from different folder/subfolders but did not find any issue ? Would you be able to share the command that raised the relative import error. Generally speaking, I would rather keep the code conventions homogeneous and use relative import everywhere.

Thank you Mathieu for taking care of (1) and (4).

[mscheltienne]

pandas is currently set in the dependencies for doc, so pip install pycrostates[doc] set it up, but the use of the [] extra-dependencies is not very straightforward for users. I think for beginner users, they should use the mne-installers, and advance users can figure out that they need to install pandas at this step (if it's not already installed by default in their work environment).
That said, I think we can find a way to not use it in the tutorial.

[vahid-sb]

@mscheltienne @vferat
The issues I have raised regarding dependencies and imports are more suggestions rather than errors. Regarding imports, I have mentioned how I run the code above. I personally prefer to avoid using relative imports, exactly because of such issues that might arise when I share the code. But it is a matter of taste and I do not expect you to change it if you prefer it this way. If nobody else has reported the issue, then it is fine. And also, I got errors only from the test scripts rather than the library modules (in which you have used relative imports repeatedly). Hence, do not worry about it.

[vahid-sb]

The issues I raised above were related to test scripts, not the library itself. I went through the library modules, one by one, checking the code. And I should say I am very impressed by how you have written the code. It is done very professionally and the code is very clean, and very well documented. I congratulate you on this impressive library. And I hope it would bring the topic of microstates to the attention of a more general audience. I will fill in the review checklist now (assuming the couple of minor issues you mention will be resolved as part of issue #58 will be resolved soon as promised.