neuroinformatics-unit / movement

Python tools for analysing body movements across space and time
http://movement.neuroinformatics.dev
BSD 3-Clause "New" or "Revised" License
77 stars 7 forks source link

matplotlib required to run examples not installed within the movement env #158

Closed antortjim closed 2 months ago

antortjim commented 3 months ago

Describe the bug The recommended installation instructions don't install all dependencies

To Reproduce

mamba create -n movement-env -c conda-forge python=3.10 pytables
mamba activate movement-env
python -m matplotlib

Expected behaviour I expected matplotlib to load as expected

Log file

python -m matplotlib
Traceback (most recent call last):
  File "/home/vibflysleep/mambaforge/envs/movement-env/lib/python3.10/runpy.py", line 187, in _run_module_as_main
    mod_name, mod_spec, code = _get_module_details(mod_name, _Error)
  File "/home/vibflysleep/mambaforge/envs/movement-env/lib/python3.10/runpy.py", line 146, in _get_module_details
    return _get_module_details(pkg_main_name, error)
  File "/home/vibflysleep/mambaforge/envs/movement-env/lib/python3.10/runpy.py", line 110, in _get_module_details
    __import__(pkg_name)
  File "/home/vibflysleep/.local/lib/python3.10/site-packages/matplotlib/__init__.py", line 161, in <module>
    from . import _api, _version, cbook, _docstring, rcsetup
  File "/home/vibflysleep/.local/lib/python3.10/site-packages/matplotlib/rcsetup.py", line 28, in <module>
    from matplotlib._fontconfig_pattern import parse_fontconfig_pattern
  File "/home/vibflysleep/.local/lib/python3.10/site-packages/matplotlib/_fontconfig_pattern.py", line 15, in <module>
    from pyparsing import (
ModuleNotFoundError: No module named 'pyparsing'

Screenshots

Computer used (please complete the following information): OS: Ubuntu 22.04.2 LTS

I think the version of matplotlib that is installed by default (3.8.3) is broken. This bug seems to be workarounded with pip instal pyparsing, but this step should be performed by pip when installing movement.

lochhh commented 2 months ago

Hi @antortjim, thank you for reporting this. matplotlib is not a required dependency for using movement and hence is not installed in the environment. At the moment, matplotlib is only required when building movement documentation locally whilst contributing to the documentation.

niksirbi commented 2 months ago

Thanks for raising this issue @antortjim. As @lochhh said, matplotlib is not currently among our dependencies, so if you follow our recommended installation instructions, it will not be installed in your environment. Currently you would have to manually add pip install matplotlib in the same environment.

That's because matplotlib is not strictly necessary for using movement, and we want to keep the installation lightweight.

That said, our examples make use of matplotlib (via xarray's built-in plotting capabilities), and I expect that most users will want to plot the data. So there's an argument to be made for making it a requirement.

Perhaps we should do what xarray does - i.e. make it an optional dependency, such that you don't get it if you do pip install movement , but you do if you run pip install movement[plot] (or pip install movement[viz] or similar). Besides, I expect that matplotlib will become a dependency down the line, as we add more plotting functionalities to movement.

Let us know if that answered your question, and if you have an opinion on the matter.

antortjim commented 2 months ago

Dear @lochhh and @niksirbi thanks for helping here. Sorry I didn't even mention why I was trying to import matplotlib. As pointed out by @niksirbi, indeed, I saw it used in the documentation. For example, this snippet taken from https://movement.neuroinformatics.dev/examples/load_and_explore_poses.html#load-and-explore-pose-tracks

from movement import sample_data
from movement.io import load_poses
file_path = sample_data.fetch_sample_data_path("SLEAP_three-mice_Aeon_proofread.analysis.h5")
ds = load_poses.from_sleap_file(file_path, fps=50)
pose_tracks = ds.pose_tracks
da = pose_tracks.sel(individuals="AEON3B_NTP", keypoints="centroid")
da.plot.line(x="time", row="space", aspect=2, size=2.5)

throws the error I pasted above (which is not user friendly). Now I understand that neither movement nor xarray require matplotlib to work. However, I think users are very likely to plot their data, and xarray does have a dedicated optional set of dependencies needed for visualization (from https://docs.xarray.dev/en/stable/getting-started-guide/installing.html#instructions: python -m pip install "xarray[viz]"). So just a quick mention that plotting requires installing xarray with viz dependencies here https://movement.neuroinformatics.dev/getting_started.html#plotting (just like they do in xarray docs) would suffice I think. Thanks again! Antonio

lochhh commented 2 months ago

Thanks @antortjim for clarifying this. This is indeed an issue since we expect users to be able to run the examples following our docs. I've edited the title of the issue to better describe this.

niksirbi commented 2 months ago

Thanks both, I agree we should remedy this. I see two alternatives:

  1. in our dependencies, we change xarray[accel] to xarray[accel,viz]. This would enable users to benefit from xarray's full plotting functionalities and would avoid the need to install any additional packages for running the examples.
  2. we keep our dependencies unchanged, but we prominently document that users have to install additional packages to reproduce the examples.

I personally lean towards 1, because most users will be using movement in conjunction with matplotlib (and maybe seaborn) anyway. Our dependencies will increase, but given that these are mature projects within the scientific Python ecosystem, we shouldn't be too wary of that.

adamltyson commented 2 months ago

I also prefer 1. We expect users to be able to use movement with the rest of the scientific Python ecosystem, and matplotlib isn't such a tricky dependency.

niksirbi commented 2 months ago

Alright, I can try to implement the (hopefully quick) fix.