BF: #596 First point of LazyTractogram is not saved
#588
Conversation
|
Much cleaner than my version. Thank you. |
Codecov Report
@@ Coverage Diff @@
## master #588 +/- ##
==========================================
+ Coverage 94.46% 94.46% +<.01%
==========================================
Files 177 177
Lines 25153 24936 -217
Branches 2684 2659 -25
==========================================
- Hits 23760 23556 -204
+ Misses 914 908 -6
+ Partials 479 472 -7
Continue to review full report at Codecov.
|
|
This PR is ready for reviews. :-) |
|
I don't have any concern with this PR; it fixes the problem and it's clean. +1 to merge. |
| try: | ||
| first_item = next(iter(self.tractogram)) | ||
| # Use the first element to check |
There was a problem hiding this comment.
I guess you could refactor this out into:
def peek_next(iterable):
next_item = next(iterable)
return next_item, itertools.chain([next_item], iterable)
There was a problem hiding this comment.
Good idea. Will do.
nibabel/streamlines/trk.py
Outdated
| @@ -423,8 +424,20 @@ def save(self, fileobj): | |||
| i4_dtype = np.dtype("<i4") # Always save in little-endian. | |||
| f4_dtype = np.dtype("<f4") # Always save in little-endian. | |||
|
|
|||
| # Make sure streamlines are in rasmm then send them to voxmm. | |||
There was a problem hiding this comment.
Could you say a bit more about this change?
There was a problem hiding this comment.
Because self.tractogram could be a LazyTractogram, peeking the first element requires to call iter() on it (i.e. start looping through the TractogramItems), but later I have to send the streamlines to voxmm, that is calling apply_affine on self.tractogram which would have been advanced by one element (because of peek_next).
The alternative would have been to create a new LazyTractogram from the iterable return by peek_next, then call the apply_affine function. I just thought it would be simpler to do the transformation first. It shouldn't change the overall logic of the method: peek element to get some data type information, apply affine and save to disk.
There was a problem hiding this comment.
Could you add something like this as an explanation in the code?
|
Yes, looks good - in it goes. |
With this PR, we now assume tractogram objects can be iterated over only once at saving time.
This PR fixes issue #586.
Thanks to @nilgoyette for providing the missing unit test and part of the solution 👍.