[neuralynx] memory improvements

[JuliaSprenger]

This PR includes the latest unittest fixes (#1025)

[pep8speaks]

Hello @JuliaSprenger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-09-09 09:05:22 UTC

[PeterNSteinmetz]

Please also see PR #973 which eliminated the creation of an object in the inner loop.

[PeterNSteinmetz]

The big question is whether this passes all the existing tests of course.

On line 288 I don't think I would compare to nb_valid for the first record. Rather I would compare to the maximum number of samples for record, which is 512, and I believe is a defined constant. There is no guarantee that the first record is full, though this is usually the case.

On line 292, should this comparison be < rather than <=. It seems like = would be an exact match to what is expected and the timestamps in the records are rounded to the nearest microsecond.

If one is going to call this 'delayed_recs' it might be best to explain why the code looks for < rather than simply != . It seems like it should never be greater than as that would imply that a sample occurred before the next sampling interval.

line 343 my sense is that this estimation of the frequency is correct, but the proof is in the tests

[CodyCBakerPhD]

This current state of this appears to fix the memory issue for me!

[JuliaSprenger]

Ready for review!
The current version still has very different performance in terms of initialization time: on windows this seems 10x slower than on linux. This should be adressed in another PR.

@PeterNSteinmetz Thanks for your earlier comments. I improved the code accordingly.

[samuelgarcia]

I don't understand this line if you reset curBlock on next line.

[JuliaSprenger]

The lines above set the parameters of the previous block before starting a new block. Basically curBlock is always pointing to the latest block and this is where the condition is met that starts a new Block. So that's why NcsSection is called here and curBlock is reassigned.

[samuelgarcia]

@JuliaSprenger : I think I don't anderstand all details of this.
I made one random comments.

Before I merge : one question.
Did you run the
benchmark_speed_read_signals() you have in neo.test.rawiotest.rawio_compliance check that on a local machine the read speed is the same as the old implementation ?

[JuliaSprenger]

Hi @samuelgarcia Thank for having a look.
I did a benchmarking check and the performance seems to be the same as before. This PR only improved memory handling during the initialization of the IO, so performance of chunk-wise reading of data should not be affected.

I also addressed your comment, I hope this is resolved now.