Re-introduced AEDAT4 and restructured file reading #59
Conversation
Jegp
changed the title
src/file/aedat4.hpp
Outdated
| // Record offsets of event packets | ||
| // TODO: add IMU + frames | ||
| auto packets = table->table(); | ||
| for (size_t i = 0; i < packets->size(); i++) { |
There was a problem hiding this comment.
Nitpick: use pre-increment? (This is a general comment, I won't insert it at every loop, haha)
for (size_t i = 0; i < packets->size(); ++i)
There was a problem hiding this comment.
Is there a reason for that? Doesn't the increment happen between loops, so the ordering doesn't matter?
There was a problem hiding this comment.
It's just that the post-increment operator creates a temporary (save the current value, increment and return the old value), while and the pre-increment doesn't (it just increments the value). So the post-increment can have a minor performance impact (if you have millions of steps) without any benefit because we are not using the pre-increment value. As I said, very much a nitpick - maybe the compiler can even optimise this out?
There was a problem hiding this comment.
Wow, I had no idea. I'll fix that and update - thank you for that information!
| n_events > 0 ? n_events : total_number_of_events; | ||
|
|
||
| uint64_t *buffer = (uint64_t *)malloc(buffer_size * sizeof(uint64_t)); |
There was a problem hiding this comment.
Er...can we use std::array here? Raw array with malloc is so...C 😄
There was a problem hiding this comment.
Hehe that's very true. I have zero experience with the std::array construct. Won't you need compile-time size values?
I guess a similar thing could be achieved with a std::vector?
One concern would be that using containers is slower, though. Do you share that or am I being paranoid?
There was a problem hiding this comment.
Yes, std::vector might be slightly slower if we are trying to squeeze every bit of performance. But I think there might be a way to handle the compile-time size of std::array. I'll run some tests to see if there are any (dis)advantages. b(^^)
There was a problem hiding this comment.
Would be fun to see what you come up with! I'll merge it for now and then we can re-evaluate. Should we perhaps open an issue?
src/file/utils.hpp
Outdated
There was a problem hiding this comment.
TODO: @cantordust to revise at a later point using std::filesystem.
src/input/file.cpp
Outdated
There was a problem hiding this comment.
Thanks for your inputs @cantordust! I fixed the comments that made immediate sense to me and asked questions about arrays and pre-increment.
src/file/aedat4.hpp
Outdated
| // Record offsets of event packets | ||
| // TODO: add IMU + frames | ||
| auto packets = table->table(); | ||
| for (size_t i = 0; i < packets->size(); i++) { |
There was a problem hiding this comment.
Is there a reason for that? Doesn't the increment happen between loops, so the ordering doesn't matter?
| n_events > 0 ? n_events : total_number_of_events; | ||
|
|
||
| uint64_t *buffer = (uint64_t *)malloc(buffer_size * sizeof(uint64_t)); |
There was a problem hiding this comment.
Hehe that's very true. I have zero experience with the std::array construct. Won't you need compile-time size values?
I guess a similar thing could be achieved with a std::vector?
One concern would be that using containers is slower, though. Do you share that or am I being paranoid?
* Restructured AEDAT4 reading to support streaming * Added aedat4 decoding to nanobind * Cleaned up file reading hierarchy * Added python tests * Updated wheel workflow Thanks to @cantordust for valuable input!
* Restructured AEDAT4 reading to support streaming * Added aedat4 decoding to nanobind * Cleaned up file reading hierarchy * Added python tests * Updated wheel workflow Thanks to @cantordust for valuable input!
I reworked the reading of files, so we now have a common
FileBasebase class, that can.read_events(n)and.stream(n), wherenrepresents a specific number of events or -1 for everything in a file. This both dramatically simplifies the C++ code but also lets us plug-and-play formats much easier.Additionally, I
aestream_filelibrary to header-only