Conversation
|
@samuelgarcia do you think this is ready to go after getting the test files? Or do we want a milestone of 0.15.0 (for after next release). |
|
I think this is ready to go. |
zm711
left a comment
zm711
left a comment
There was a problem hiding this comment.
I think we should do a bit of a rewrite of this RawIO in general. Some of the variable naming is hard to follow and this low level seeking is also not super easy to follow. We should add some more comments to what is happening at the steps. But that can always be a future PR.
| # "TRONCA" zone define segments | ||
| zname2, pos, length = zones["TRONCA"] | ||
| f.seek(pos) | ||
| max_segments = 100 |
There was a problem hiding this comment.
where does this 100 come from. We probably need a comment for any magic number.
There was a problem hiding this comment.
I think this magic number come from other implemtation in matlab. Maybe this should be exposed, I guess this is to avoid infinite while on some dataset.
| max_segments = 100 | ||
| self.info_segments = [] | ||
| for i in range(max_segments): | ||
| seg_start = int(np.frombuffer(f.read(4), dtype="u4")[0]) |
There was a problem hiding this comment.
Same here. Could we get a comment for why we are reading 4 at a time?
There was a problem hiding this comment.
4 is 4bytes for "u4"
There was a problem hiding this comment.
Sorry I meant why are you doing 'u4'? why not some other size for reading. How do you know reading u4 gets us what we need instead of some other size? If we do a comment then we know that either 1) this size is documented or 2) you are doing this empirically.
| } | ||
|
|
||
| # "TRONCA" zone define segments | ||
| zname2, pos, length = zones["TRONCA"] |
There was a problem hiding this comment.
Light recommendation to have better name for pos :)
There was a problem hiding this comment.
outch it was already everywhere
There was a problem hiding this comment.
It's not crucial here :) But if any micromed issues come up I will send them your way.
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
|
To move forward with this I think you need to merge main into this branch to fix the CI/RTD builds. Then we can really see how the tests are doing :) |
3 participants
I though that micromed was mono segment but apparently it can be multi segment.
Here the implementation.
This include a test that need theses two files to be merge in gin https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/143