Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Miniscope Extractor Naming #374

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Refactor Miniscope Extractor Naming #374

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
daf62dc
rename legacy extractor
h-mayorquin Oct 9, 2024
9e9dfcf
Merge branch 'main' into new_miniscope_interface
weiglszonja Oct 12, 2024
d4bc98d
docstring and name correction
h-mayorquin Oct 17, 2024
f6ec161
name change on init
h-mayorquin Oct 17, 2024
bdd857e
Merge branch 'main' into new_miniscope_interface
weiglszonja Oct 22, 2024
c194d02
changelog
h-mayorquin Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/roiextractors/extractors/miniscopeimagingextractor/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@
An ImagingExtractor for the Miniscope video (.avi) format.
"""

from .miniscopeimagingextractor import MiniscopeImagingExtractor
from .miniscopeimagingextractor import MiniscopeImagingExtractor, MiniscopeMultiSegmentExtractor
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo? This is still an imaging extractor and not a segmentation right?

Copy link
Collaborator Author

@h-mayorquin h-mayorquin Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

[EDIT]

Sorry, no, it was nos a typo.

I am thinking on the concept of multi segment in spikeinterface when a session of the same data is divided with possible gaps in between. That said, it is missing the imaging word on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@weiglszonja thanks for pointing this out. Now that I think about it, MultiSegment is also a bad name because in Spikeinterface multi-segment recordings do not extract the different recordings within the session as a single continuous recording. I settled for MiniscopeMultiRecordingImagingExtractor to avoid this possible confusion.

Can you take a look at the new naming and the new docstrings to see if it makes sense?

h-mayorquin marked this conversation as resolved.
Show resolved Hide resolved
24 changes: 14 additions & 10 deletions src/roiextractors/extractors/miniscopeimagingextractor/miniscopeimagingextractor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@
from ...extraction_tools import PathType, DtypeType, get_package


class MiniscopeImagingExtractor(MultiImagingExtractor): # TODO: rename to MiniscopeMultiImagingExtractor
class MiniscopeMultiSegmentExtractor(MultiImagingExtractor):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class MiniscopeMultiSegmentExtractor(MultiImagingExtractor):
class MiniscopeMultiImagingExtractor(MultiImagingExtractor):

"""An ImagingExtractor for the Miniscope video (.avi) format.

This format consists of video (.avi) file(s) and configuration files (.json).
One _MiniscopeImagingExtractor is created for each video file and then combined into the MiniscopeImagingExtractor.
One _MiniscopeSingleVideoExtractor is created for each video file and then combined into the MiniscopeImagingExtractor.
"""

extractor_name = "MiniscopeImaging"
extractor_name = "MiniscopeMultiImaging"
is_writable = True
mode = "folder"

Expand Down Expand Up @@ -58,24 +58,28 @@ def __init__(self, folder_path: PathType):

imaging_extractors = []
for file_path in miniscope_avi_file_paths:
extractor = _MiniscopeImagingExtractor(file_path=file_path)
extractor = _MiniscopeSingleVideoExtractor(file_path=file_path)
extractor._sampling_frequency = self._sampling_frequency
imaging_extractors.append(extractor)

super().__init__(imaging_extractors=imaging_extractors)


class _MiniscopeImagingExtractor(ImagingExtractor):
"""An ImagingExtractor for the Miniscope video (.avi) format.
# Temporal renaming to keep backwards compatibility
h-mayorquin marked this conversation as resolved.
Show resolved Hide resolved
MiniscopeImagingExtractor = MiniscopeMultiSegmentExtractor
h-mayorquin marked this conversation as resolved.
Show resolved Hide resolved


class _MiniscopeSingleVideoExtractor(ImagingExtractor):
"""An auxiliar extractor to get data from a single Miniscope video (.avi) file.

This format consists of a single video (.avi) file and configuration file (.json).
Multiple _MiniscopeImagingExtractor are combined into the MiniscopeImagingExtractor for public access.
This format consists of a single video (.avi)
Multiple _MiniscopeSingleVideoExtractor are combined into the MiniscopeImagingExtractor for public access.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Multiple _MiniscopeSingleVideoExtractor are combined into the MiniscopeImagingExtractor for public access.
Multiple _MiniscopeSingleVideoExtractor are combined into the MiniscopeMultiImagingExtractor for public access.

"""

extractor_name = "_MiniscopeImaging"
extractor_name = "_MiniscopeSingleVideo"

def __init__(self, file_path: PathType):
"""Create a _MiniscopeImagingExtractor instance from a file path.
"""Create a _MiniscopeSingleVideoExtractor instance from a file path.

Parameters
----------
Expand Down
Loading