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

ENH Tractography::Writer: Add tractogram timestamp to weights file #1849

Open
Lestropie opened this issue Dec 15, 2019 · 3 comments
Open

ENH Tractography::Writer: Add tractogram timestamp to weights file #1849

Lestropie opened this issue Dec 15, 2019 · 3 comments
Labels

Comments

@Lestropie
Copy link
Member

@Lestropie Lestropie commented Dec 15, 2019

Idea came from forum thread, and sort of follows #1603.

If a matrix / vector text file can contain arbitrary key-value entries in comment lines at the head of the file, there's no reason why the timestamp of the corresponding track file can't be written, and then on read compared to the open track file just as is done for TSFs. This has the chance of catching instances where the track file and weights file don't actually correspond to one another.

@Lestropie Lestropie added the wishlist label Dec 15, 2019
@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 15, 2020

Happy with the concept. Can we however downgrade these types of issues to warnings rather flat-out errors? I've been caught out when trying to load a TSF file generated from an old TCK file that didn't have a timestamp...

@Lestropie

This comment has been minimized.

Copy link
Member Author

@Lestropie Lestropie commented Jan 15, 2020

If there's no timestamp present it'd obviously need to be no higher than warning-level. Even mismatched timestamps are less consequential here: with TSF the requirements for exact per-vertex matching is much more strict so we reject anything that can't be verified, whereas here when it's only the number of streamlines that should ideally be matched the consequences are not so great.

I think it'd need to be no higher than warning-level even on an actual mismatch in this case, since e.g. you could run something like tckresample and the original weights file would still be applicable to the output. Indeed tckresample doesn't currently permit input / output weight files, so the warning would in fact be unavoidable. Based on that, actually using the timestamp in weights files might be too prohibitive; the fact of a mismatch could certainly be concatenated to any error string generated, but maybe it shouldn't be used to make decisions about such.

Since the weights file is now loaded fully immediately using load_vector(), the size should be checked immediately against the track file's count field; maybe that's where a warning should be generated from instead. I could also write count to the comment section of weights files for the sake of it.

As far as TSFs, we could downgrade from exception to warning in the absence of a timestamp if you'd prefer.

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 15, 2020

Happy with all of the above. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.