danjgale / surfplot

A package for publication-ready brain surface figures
https://surfplot.readthedocs.io/en/latest/
Other
53 stars 14 forks source link

Small bug when loading fslr surfaces on windows #13

Closed rscgh closed 1 year ago

rscgh commented 2 years ago

Hi everyone,

thanks for providing this package. I was just trying to plot some data on an inflated surface and realized that in Windows, loading the fslr surfaces as shown in Tutorial 1 does not work. Specifically calling:

from surfplot import Plot
from neuromaps.datasets import fetch_fslr
surfaces = fetch_fslr() 
lh, rh = surfaces['inflated']
#lh == WindowsPath('[C:/Users/rsc/neuromaps-data/atlases/fsLR/tpl-fsLR_den-32k_hemi-L_inflated.surf.gii]()
p = Plot(lh)

leads to a ValueError: ValueError: Surface be a path-like string, an instance of BSPolyData, or None

I believe the check in Line 18 of plotting.py leads to the error beeing called, as it expects a PosixPath, but gets a WindowsPath: if isinstance(surf, (str, pathlib.PosixPath)):

To circumvent that, I can cast the path to str or just load the surface myself (using the respective surfplot function):

p = Plot(str(lh))
# or
from brainspace.mesh.mesh_io import read_surface
bspoly_lh = read_surface(str(lh)) 
# bspoly_lh == BSPolyData object at 0x0000016289604130 [Wrapping a vtkmodules.vtkCommonDataModel.vtkPolyData(0x00000162F1A3D490]
p = Plot(bspoly_lh )

Either method results in no error, and I can plot everything as usual.

A simple fix could be changing the required object type to pathlib.Path (in case there are no specific reasons why a posixPath should be required).

Note: this is also not an issue when using load_conte69() as this function directly returns two BSPolyData objects.

danjgale commented 2 years ago

Hi @rscgh, thanks for pointing this out! I agree that pathlib.Path could completely avoid this issue.

I honestly don't know why I used PosixPath specifically in this instance. In any case, I'll add it to the bug-list to fix for the next release. Cheers!

danjgale commented 1 year ago

This should be fixed now with #20