search

tee-ar-ex / trx-python

Python implementation of the TRX file format
https://tee-ar-ex.github.io/trx-python/
BSD 2-Clause "Simplified" License
22 stars 15 forks source link

Saving offsets with N+1 elements #33

Closed frheault closed 2 years ago

frheault commented 2 years ago

This is a test following @neurolabusc suggestion to have the TRX specifications of offsets closer to the one of VTK. This means the offsets have N+1 elements (nb_streamlines+1) and the last one is equal to nb_vertices.

This does not change much in terms of internal use for Nibabel except we ignore the last element when loading and add the last element when saving.

This is simply a test to see what had to be changed. This also means everything testing TRX file generated would have to be modified and re-uploaded.

That was @neurolabusc suggestion and he wrote me this before:

As an aside, I would urge the TRX specification to follow the VTK lead in solving the fence post problem for the offsets. The fact that the length of the final streamline can not be determined from the offsets alone, but must be inferred from the number of positions is awkward. Fast code really needs this to avoid the penalty of range check conditionals. Therefore, I really think storing NB_STREAMLINES + 1 offsets is correct. VTK sets a precedence for this and it makes any code supporting this format cleaner, easier to maintain and simpler to understand.

As the proposal is still up for debate and modification, I wanted to show it is easy to change/support for him. But I would leave it to everyone to decide if we change the specification.

Lestropie commented 2 years ago

Conceptually, I think that this makes sense. Implementations that are designed around tracking the total vertex count can still work just by ignoring the additional data, but this facilitates more computationally efficient implementations.And as @neurolabusc says, there's a precedent set in a far more mature format.

frheault commented 2 years ago

@arokem Any opinions on this? This is a simple change in code, but will require an update of the files on Figshare (if merged). I will have time for that with good wifi after my vacation.

This could be a good justification to create a tag (version 0.1) so we can sync all repo on the same of the spec. That way the trx-cpp, trx-js, trx-python would all be version 0.1 only if they all follow the trx-spec 0.1.

This way if we do a minor change to the spec later, that would be 0.2 and we can follow which version on Figshare works with which tag on Github.

What do you think (about the versioning and this PR)?

arokem commented 2 years ago

+1 to the suggested change!

Along the way, we've run here into the issue of versioning the test data, which would fall out of spec... I think that it's actually possible to do in Figshare (based on reading this page: https://help.figshare.com/article/can-i-edit-or-delete-my-research-after-it-has-been-made-public). I think that we'd need to document somewhere in the code which version of the data we are reading.