ENH: Add write_morph_data to freesurfer.io #414
Conversation
STY: Conform to conventions of other freesurfer.io functions
|
LGTM |
| @@ -160,6 +160,29 @@ def read_morph_data(filepath): | |||
| return curv | |||
|
|
|||
|
|
|||
| def write_morph_data(filepath, values): | |||
| """Write out a Freesurfer morphometry data file. | |||
There was a problem hiding this comment.
Mention parameters in first line? Something like
Write freesurfer morphometry data `values` to file `filepath`
|
Looking at |
|
Added |
Check bounds on values to be written Add fnum parameter to match write_curv.m
Squeeze array to simplify dimension test
|
New tests added. Subtle bug for vectors of shape |
| """ | ||
| magic_bytes = np.array([255, 255, 255], dtype=np.uint8) | ||
|
|
||
| array = np.asarray(values).astype('>f4').squeeze() |
There was a problem hiding this comment.
Just checking, but you want to allow arrays shape (1, 1, 20, 1, 1)?
There was a problem hiding this comment.
Is there a good reason not to?
There was a problem hiding this comment.
You could imagine that user was making a mistake, passing in a multidimensional array, that just happened to have only one dimension not length 1. I mean, maybe you actually want to accept any of shape (N,), (1, N), (N, 1), but not other shapes. As in if array.ndim > 2 or (array.ndim == 2 and not 1 in array.shape):
Actually maybe array is a bit confusing as name (looks like np.array) ...
There was a problem hiding this comment.
I suppose. Maybe it makes more sense to let people do their own squeezing, if that's what they want.
I would say allow (N, 1, 1) as well, though, since that's the default shape coming in from MGH or surface Nifti files.
Coercing vector to float before checking size could cause memory blow-up
|
|
||
| vector = np.asarray(values) | ||
| vnum = np.prod(vector.shape) | ||
| if vector.shape not in ((vnum,), (vnum, 1), (1, vnum), (vnum, 1, 1)): |
|
Great - thanks for your patience. |
|
Oops - Appveyor failure ... |
|
32bit Windows issue: https://ci.appveyor.com/project/Eric89GXL/nibabel/build/1.0.587/job/xrwxr6jdbj3s4vj9#L876
Not sure where to start with that one. |
| np.zeros(shape), big_num) | ||
| if ctypes.c_long is not ctypes.c_int32: | ||
| assert_raises(ValueError, write_morph_data, 'test.curv', | ||
| strided_scalar((big_num,))) |
There was a problem hiding this comment.
Alternatives, if this is ugly:
- Check
np.iinfo(ctypes.c_long) try: strided_scalar((big_num,))
There was a problem hiding this comment.
How about:
if np.dtype(np.long) != np.dtype(np.int32): # Windows 32-bit overflows Python int
There was a problem hiding this comment.
Doesn't seem to capture the case for PYTHON_VERSION=2.7, PYTHON_ARCH=32.
| assert_raises(ValueError, write_morph_data, 'test.curv', | ||
| np.zeros(shape), big_num) | ||
| # Windows 32-bit overflows Python int | ||
| if np.dtype(np.long) != np.dtype(np.int32): |
There was a problem hiding this comment.
Whoops. How about np.dtype(int) != np.dtype(np.int32) ?
|
Looks good on the tests - will merge unless I hear otherwise tomorrow. |
|
Just squashed the final two commits. Should be good to go. Thanks for the review. |
|
Great - thanks a lot. |
ENH: Add write_morph_data to freesurfer.io The first commit was in the midst of #249 and causing PEP8 warnings. Not otherwise relevant to Cifti, so I'm submitting as its own PR. Fixed the style and brought a little more in line with freesurfer.io functions.
ENH: Add write_morph_data to freesurfer.io The first commit was in the midst of nipy#249 and causing PEP8 warnings. Not otherwise relevant to Cifti, so I'm submitting as its own PR. Fixed the style and brought a little more in line with freesurfer.io functions.
The first commit was in the midst of #249 and causing PEP8 warnings. Not otherwise relevant to Cifti, so I'm submitting as its own PR. Fixed the style and brought a little more in line with
freesurfer.iofunctions.