Skip to content

Extend si.load capabilities - #3651

Merged
samuelgarcia merged 26 commits into
SpikeInterface:mainfrom
alejoe91:more-loading
Feb 7, 2025
Merged

Extend si.load capabilities#3651
samuelgarcia merged 26 commits into
SpikeInterface:mainfrom
alejoe91:more-loading

Conversation

@alejoe91

Copy link
Copy Markdown
Member

This PR extends the si.load magic function to load also SortingAnalyzers.

In addition, a better logic is implemented to load zarr files, both locally and remotely, including trying to open the folder with different options: zarr.open/zarr.open_consolidated and anon False/True for remote assets. This simplifies handling of zarr opening everywhere

@alejoe91 alejoe91 added this to the 0.102.0 milestone Jan 29, 2025
@alejoe91
alejoe91 requested a review from samuelgarcia January 29, 2025 14:13
@alejoe91 alejoe91 added the core Changes to core module label Jan 29, 2025
Comment thread pyproject.toml Outdated
"pytest",
"pytest-dependency",
"psutil",
"s3fs", # for streaming tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big one for core tests no ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to not have core tests that do downloading no ?
Because the matrix of version X OS is big and failure probability is bigger when need external URL.
I would move the S3 stuff in test_extractor somewhere.

@samuelgarcia

Copy link
Copy Markdown
Member

Good idea but I am not sure to like the design with try except.
We need to do clear check on json files or zarr attribute (also) json for information on chich object it is.

@samuelgarcia

Copy link
Copy Markdown
Member

We could also add the Motion object.

@samuelgarcia

Copy link
Copy Markdown
Member

And also Template

@samuelgarcia

Copy link
Copy Markdown
Member

@alejoe91 : I refactor everything to have more general case.
This looks big but this is an easier design which avoid nested if else evrywhere.
You will see that there are 2 steps guess the object and then loading.
This is more easy to maintains.

Comment on lines +296 to +298
# This case shoudl deprecated soon because the read_zarr is ultra ambiguous
# just testing if the zarr contains unit_ids or channel_ids but many object also contains it (see template)!!!!
from .zarrextractors import read_zarr

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alejoe91 : any idea how to avoid using this read_zarr stuff ?
If think that read_zarr should deprecated because this is too ambiguous.

storage_options = backend_options.get("storage_options", {})

zarr_root = zarr.open_consolidated(str(folder), mode="r", storage_options=storage_options)
zarr_root = super_zarr_open(str(folder), mode="r", storage_options=storage_options)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alejoe91 : Here, is it a problem that the zarr_root could be eventually not consolidated ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could for some older assets. Plus in append mode we can't use consolidated

@samuelgarcia

Copy link
Copy Markdown
Member

@alejoe91 : really cool.
The only stuff we should change is the s3fs for core testing. I would move it to a less frequent testing like extrator even if it is in the core because the core tests is havilly launched and any eternal downloading would make it more slow I think.

@alejoe91

alejoe91 commented Feb 6, 2025

Copy link
Copy Markdown
Member Author

@alejoe91 : really cool. The only stuff we should change is the s3fs for core testing. I would move it to a less frequent testing like extrator even if it is in the core because the core tests is havilly launched and any eternal downloading would make it more slow I think.

I'll add it to the steaming extractors tests

@samuelgarcia
samuelgarcia merged commit 9ed4974 into SpikeInterface:main Feb 7, 2025
@alejoe91
alejoe91 deleted the more-loading branch March 20, 2026 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes to core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants