Refactor Miniscope Extractor Naming #374
Conversation
h-mayorquin
changed the title
| @@ -11,4 +11,4 @@ | |||
| An ImagingExtractor for the Miniscope video (.avi) format. | |||
| """ | |||
|
|
|||
| from .miniscopeimagingextractor import MiniscopeImagingExtractor | |||
| from .miniscopeimagingextractor import MiniscopeImagingExtractor, MiniscopeMultiSegmentExtractor | |||
There was a problem hiding this comment.
Is this a typo? This is still an imaging extractor and not a segmentation right?
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
@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?
src/roiextractors/extractors/miniscopeimagingextractor/__init__.py
Outdated
Show resolved
Hide resolved
| @@ -18,14 +18,14 @@ | |||
| from ...extraction_tools import PathType, DtypeType, get_package | |||
|
|
|||
|
|
|||
| class MiniscopeImagingExtractor(MultiImagingExtractor): # TODO: rename to MiniscopeMultiImagingExtractor | |||
| class MiniscopeMultiSegmentExtractor(MultiImagingExtractor): | |||
There was a problem hiding this comment.
| class MiniscopeMultiSegmentExtractor(MultiImagingExtractor): | |
| class MiniscopeMultiImagingExtractor(MultiImagingExtractor): |
src/roiextractors/extractors/miniscopeimagingextractor/miniscopeimagingextractor.py
Outdated
Show resolved
Hide resolved
| 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. |
There was a problem hiding this comment.
| Multiple _MiniscopeSingleVideoExtractor are combined into the MiniscopeImagingExtractor for public access. | |
| Multiple _MiniscopeSingleVideoExtractor are combined into the MiniscopeMultiImagingExtractor for public access. |
src/roiextractors/extractors/miniscopeimagingextractor/miniscopeimagingextractor.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you @h-mayorquin for spearheading this task; before approving this the typos should be addressed but the rest makes sense to me.
|
Thanks @weiglszonja, I added what I think are better docstrings and settled for |
|
@h-mayorquin in fact, can you also update the changelog? this seems like an important name change to be included in there. After that you can merge I think :) |
|
Thanks, I added a warning when using the current structure warning that the signature might change and telling the user that they should use the other extractor. |
First PR in the direction of #356
It does two things:
MiniscopeImagingExtractortoMiniscopeMultiSegmentExtractorwhich I think better encompasses its scope and purpose. It then does a renaming to keep (for the moment) backward compatibility._MiniscopeImagingExtractorto_MiniscopeSingleVideoExtractorfor similar reasons.These two changes pave the road to implement a future
MiniscopeImagingExtractorin the lines of the comments in f #356