Skip to content

Make binary recording memmap efficient #1741

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
Closed
h-mayorquin wants to merge 3 commits into from
Closed

Make binary recording memmap efficient #1741

h-mayorquin wants to merge 3 commits into from

Conversation

@h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Jun 23, 2023

In the context of #1602 and NeuralEnsemble/python-neo#1265.

This does not go all the way to use native python buffers which is what we have done in those PR but it moves closer in that direction. I added a further test.

@h-mayorquin h-mayorquin added the core Changes to core module label Jun 23, 2023
@h-mayorquin h-mayorquin self-assigned this Jun 23, 2023
@h-mayorquin h-mayorquin mentioned this pull request Jun 23, 2023
Merged
5 tasks
@h-mayorquin h-mayorquin marked this pull request as ready for review June 23, 2023 15:00
samuelgarcia
samuelgarcia reviewed Jun 23, 2023
View reviewed changes
src/spikeinterface/core/binaryrecordingextractor.py
channel_indices: Union[List, None] = None,
) -> np.ndarray:
traces = self._timeseries[start_frame:end_frame]
data = np.memmap(self.datfile, self.dtype, mode="r", offset=self.file_offset, shape=self.shape)
Copy link
Member

@samuelgarcia samuelgarcia Jun 23, 2023

Choose a reason for hiding this comment

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

Hi Ramon.
We should use the same trick as in neo. Open the file in init an d create the memmap on the fly.
open/close on network drive has a cost!

Copy link
Collaborator Author

@h-mayorquin h-mayorquin Jun 24, 2023

Choose a reason for hiding this comment

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

I made the two of them but I wanted to make smaller PRs:

#1742

@DradeAW
Copy link
Contributor

DradeAW commented Jun 24, 2023

I don't understand why opening the memmap on get_traces is more efficient?

Opening and closing comes at a cost, but once opened it should only be a pointer to the file?
How is this more efficient?

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Jun 24, 2023 •
edited
Loading

@DradeAW
Long story short, the memmaps are not released (and theres is no API to release them) and lead to a linear memory increase. This is one way that we find of avoiding this problem. Opening memmaps and files is not that expensive when compared to most operations except on the network case which is what Sam is talking about.

@h-mayorquin h-mayorquin mentioned this pull request Jun 24, 2023
Merged
@DradeAW
Copy link
Contributor

DradeAW commented Jun 24, 2023

the memmaps are not released

Interesting, I always thought that calling memmap.close() inside __del__ took care of this issue. I'm gonna have to look into this for my own code!

Thank you :)

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Jun 24, 2023

You can read here why there is no unified interface:

numpy/numpy#13510

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Jun 24, 2023

I am thinking on writing a blogpost about it but June is a very busy time for contracts. Maybe next month.

@samuelgarcia samuelgarcia added this to the 0.98.0 milestone Jun 27, 2023
@samuelgarcia
Copy link
Member

samuelgarcia commented Jun 27, 2023

I propose to close this and merge #1742 instead.

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Jun 27, 2023

Fine by me, I added the close flag on the other PR.

@h-mayorquin h-mayorquin deleted the make_binary_recording_memmap_efficient branch June 27, 2023 09:41
@h-mayorquin h-mayorquin restored the make_binary_recording_memmap_efficient branch June 27, 2023 09:41
@h-mayorquin h-mayorquin deleted the make_binary_recording_memmap_efficient branch June 27, 2023 09:41
@alejoe91 alejoe91 removed this from the 0.98.0 milestone Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelgarcia samuelgarcia samuelgarcia left review comments

Assignees

@h-mayorquin h-mayorquin

Labels

core Changes to core module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@h-mayorquin @DradeAW @samuelgarcia @alejoe91