Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saving offsets with N+1 elements #33

Merged
merged 8 commits into from
Oct 5, 2022

Conversation

frheault
Copy link
Collaborator

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
Copy link

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.

@Lestropie Lestropie mentioned this pull request Jun 20, 2022
@frheault
Copy link
Collaborator Author

@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
Copy link
Collaborator

arokem commented Jun 22, 2022

+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.

@frheault frheault changed the title Saving offsets with N+1 elements (WIP) Saving offsets with N+1 elements Sep 27, 2022
('https://figshare.com/ndownloader/files/35709764',
'511c59a3ee8c16b4be0ef815b889e939')
('https://figshare.com/ndownloader/files/37624151',
'd9f220a095ce7f027772fcd9451a2ee5')
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to record here which version of the figshare data is being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment at the end of each line with the version and date from figshare.

('https://figshare.com/ndownloader/files/35617193',
'7d7082f7f2e07cb39e2fef13107af169'),
('https://figshare.com/ndownloader/files/37624154',
'b847f053fc694d55d935c0be0e5268f7'), # V1 (27.09.2022)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also supposed to be "V2"?

Suggested change
'b847f053fc694d55d935c0be0e5268f7'), # V1 (27.09.2022)
'b847f053fc694d55d935c0be0e5268f7'), # V2 (27.09.2022)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no DSI.zip before, so this is the first version.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Sounds good!

@arokem arokem merged commit c557539 into tee-ar-ex:master Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants