Skip to content

NF: allow truncated last track in trackvis file #346

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

Merged
merged 1 commit into from
Sep 19, 2015

Conversation

matthew-brett
Copy link
Member

From report on mailing list by Carolyn D. Langen - trackvis reader was giving
TypeError when trying to read file where last track was shorter than declared
in the 'n_points' data.

Allow this situation for the last track with new 'errors' kwarg.

This is a slight API break because:

  • We are now raising a DataError if there are too few streamlines in the
    file, instead of a HeaderError;
  • We are raising a DataError if the track is truncated, rather than a
    TypeError when trying to create the points array.

@matthew-brett
Copy link
Member Author

Question - is errors={'strict', 'lenient'} the best way to name the kwarg?

What do people feel about the API break? Break is that this code returns different errors to the previous code.

@matthew-brett matthew-brett force-pushed the more-lenient-trackvis branch 2 times, most recently from d92c386 to b54671f Compare September 5, 2015 19:12
@effigies
Copy link
Member

effigies commented Sep 6, 2015

To avoid an API break, one option is errors={'strict', 'lenient', 'original'}, make 'original' the default and raise a deprecation warning. (Where 'original' could also be some more sensible name.)

If breaking the API is fine, and only two modes are needed, then any reason not to do strict={True, False}?

@matthew-brett
Copy link
Member Author

I was thinking that we might want to allow different set of errors to pass, in the future, which might have different strings, or even lists specifying the errors that can pass. Might be a bit much though.

From report on mailing list by Carolyn D. Langen - trackvis reader was
giving TypeError when trying to read file where last track was shorter
than declared in the 'n_points' data.

Allow this situation for the last track with False argument to new
`strict` kwarg of the trackvis `read` function.

This is a slight API break because:

* We are now raising a DataError if there are too few streamlines in the
  file, instead of a HeaderError;
* We are raising a DataError if the track is truncated, rather than a
  TypeError when trying to create the points array.
@matthew-brett
Copy link
Member Author

OK - I went for strict=True|False as you suggested. We can always tune this by allowing strings or lists as values in some future extension.

I think the API change is OK, it's pretty minor, and I think the errors should be DataError.

@matthew-brett
Copy link
Member Author

Is this one OK to merge? Any other comments here?

@effigies
Copy link
Member

LGTM.

Edit: I'm assuming you tried this on the .trk files the reporter posted to the list? It'd be good to get a confirmation from her that it works for her use cases, but don't know you can count on that.

@matthew-brett
Copy link
Member Author

Chris - yes - this does load the file the reporter shared...

matthew-brett added a commit that referenced this pull request Sep 19, 2015
MRG: allow truncated last track in trackvis file

From report on mailing list by Carolyn D. Langen - trackvis reader was giving
TypeError when trying to read file where last track was shorter than declared
in the 'n_points' data.

Allow this situation for the last track with new 'strict' kwarg.

This is a slight API break because:

We are now raising a DataError if there are too few streamlines in the file, instead of a HeaderError;
We are raising a DataError if the track is truncated, rather than a TypeError when trying to create the points array.
@matthew-brett matthew-brett merged commit 3bc31e9 into nipy:master Sep 19, 2015
grlee77 pushed a commit to grlee77/nibabel that referenced this pull request Mar 15, 2016
MRG: allow truncated last track in trackvis file

From report on mailing list by Carolyn D. Langen - trackvis reader was giving
TypeError when trying to read file where last track was shorter than declared
in the 'n_points' data.

Allow this situation for the last track with new 'strict' kwarg.

This is a slight API break because:

We are now raising a DataError if there are too few streamlines in the file, instead of a HeaderError;
We are raising a DataError if the track is truncated, rather than a TypeError when trying to create the points array.
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.

2 participants