scan_ncs_files() not lazy?

[CodyCBakerPhD]

@samuelgarcia @PeterNSteinmetz

Hello - as a follow up to the fix provided by PR #957 which resolved the basic access issue raised in Issue #959, I am noticing behavior similar to old Issue #735.

Simple summary: I'm trying to utilize the single-file mode for Neuralynx through spikeextractors by making a list of NeurlynxRecordingExtractors for each file (safer than using the dirname directly) as follows

test = [NeuralynxRecordingExtractor(filename=filename) for filename in neuralynx_files]

with neuralynx_files being a list of several .ncs file paths. While this works fine from a purely functional standpoint (all data and metadata is readily reported and appears accurate), I noticed that the more files I included in neuralynx_files, the slower the loading was and the more RAM was being used. On the upper end (64 files, up to 1 GB each), my own system would not be able to load in all the files I need to use.

Via a memory profiler I was able to track the memory usage through the RawNeoIO in spikeextractors to the parse_header() command in neo itself, and from there down to the call to scan_ncs_files(), which appears to be loading in all the data for that .ncs file rather than utilizing lazy access through memory maps and such.

Is this intended, or a bug caused by the new filename feature?

[PeterNSteinmetz]

Not a new bug. This can likely be optimized. When the file does not appear to be one contiguous set of records due to gaps in the time stamps the start of every record in the file must be scanned to determine the appropriate segmenting of those records.

There may be a faster and more memory efficient way though I don’t see why the bits which are read should not be garbage collected.

This version is correct. Perhaps the next version can be faster or have a better memory footprint.

[samuelgarcia]

If I remember correctly (because I haven't refactor neuralynxio for a while):

• gap detection cost a lot (memory+cpu)
• the gap detection is done on the first channel and then gaps are checked if indentical across channels
  which is faster than redetecting for every channels

@PeterNSteinmetz : is it still the case ?

If yes, so reading channel per channel should slower and should use mroe ram.

[PeterNSteinmetz]

The new algorithm is more complicated to handle a bunch of different cases that arise with different programs generating these .Ncs files. Likely best to see the code and comments to understand them all, but in terms of the items addressed here by @samuelgarcia, here is a brief summary of the new algorithm.

First check the first and last record of the file (possible because records are fixed physical length). If it appears all the records are one contiguous segment of records based on the sampling rate in the header and the first and last time stamps, then assume there is one contiguous segment and setup the headers for that.

This is by far the most common case, but can be wrong in a number of ways: sampling rate in header not an exact match for the actual clock being used by the recording system, gaps when the recording was started and stopped, etc. In those cases, each record will have to be checked to determine the appropriate contiguous segments.

[bendichter]

So it sounds like in our case the algorithm is detecting multiple discontinuous segments? @CodyCBakerPhD, is that appropriate for the file you are reading?

[PeterNSteinmetz]

Had a brief look at this and the memory from the files is loaded as a memmap. But if the record scan needs to take place, then a small object is created for the data at the start of each record and of course that data is loaded from the memmap. None of this is supposed to be retained however, so I would not expect a memory leak.

[CodyCBakerPhD]

@PeterNSteinmetz I agree this should not be happening - the memory profiler was able to track the issue further down to the CscRecordHeader in line #270 in the _parseForMaxGap function.

Attempting to delete the hdr object both in the for loop (after use) and outside fails to solve the issue, but deleting the io object after it finishes initializing will free up the space.

@JuliaSprenger is looking into the issue now I think, and has access to the data for testing

[PeterNSteinmetz]

@JuliaSprenger Just pushed PR #973 which passes all tests and no longer creates any objects in the loop. Hopefully this will eliminate this apparent memory issue. @CodyCBakerPhD don't know if you can pull that directly and test for your use case?

[CodyCBakerPhD]

@PeterNSteinmetz Yeah, I am able to pull from your fork for testing. The read performs significantly faster (not the biggest concern) but the memory issue is still there so more investigation will be needed. If @JuliaSprenger is able to replicate the issue that would probably be the most direct path to solving it as I don't have the best knowledge of these internal workings.

[PeterNSteinmetz]

@CodyCBakerPhD This strikes me as puzzling. The code now creates one memmap per file. That is stored in the RawIO object for each file included. If the memory being used is proportional to the number of files open and deleting the IO object obviates the memory problem, that suggests that the memmap is not very well optimized or not unloading its cache properly.

If the solution cannot be found while using memmap, I can record this to just read and scan the binary file while loading very little into memory. Indeed, the original Java version of this treated the files exactly that way,

[PeterNSteinmetz]

@CodyCBakerPhD Can you send me one of the files in question? My email is ndoc3@steinmetz.org and we can work out details. I would like to have a look at the record structure and the headers. Or we can use a pull request on ephy_testing_data if you prefer.

[CodyCBakerPhD]

@PeterNSteinmetz OK I've shared the private (non-public) data structure with you. Hopefully we can pin this down

[PeterNSteinmetz]

@CodyCBakerPhD how do I access it? I would like to get a copy of one of the actual binary files under discussion.