MRG: fix bug in get_fdata return value #638
Conversation
There was too much indentation, making the code harder to follow. Break the code up into helper methods. Remembering (after forgetting) that method names beginning with `validate` will get called automatically by the validator post-processing.
Make dtypes explicit for return, in order to allow sub-classes to override the storable dtypes.
The `get_fdata` method should return the contained array if the array is the correct (matching) floating point type. This was not happening because of I used the `astype` method, which, by default, does a copy. Fix and test.
|
Looks like Windows+3.4 in particular doesn't like this. From a quick glance, it's unclear why these issues are raising here and not in other cases. |
|
Also, as a point of information, you've previously said that |
|
Yeah - that Appveyor error is very odd. Here's what I believe to be the same code, passing: https://ci.appveyor.com/project/matthew-brett/nibabel/build/1.0.17 . I couldn't replicate the test error on the Appveyor machines, attached via remote desktop. For the always copy comment, what I think I meant was, it always copies if it's loaded from disk. The previous tests seemed to imply that I had planned to return the internal array object only if it was already the given datatype - it's just that they weren't testing that, because they were not testing creating array images with float64 data. If you think that's the wrong thing to do, we can revisit. The returned array won't behave like a copy in all circumstances, because of the caching. So, although always returning a copy might be more consistent, it does soak up a lot of extra memory, and the API is already not consistent with respect to copies, because of the caching. |
|
Oh, gotcha. That makes sense. I'm rerunning the AppVeyor build to see if it does replicate. Maybe it was just a weird cache. |
|
It passed. Who knows what was up with that. I'm 👍 to merge. |
|
@yarikoptic I assume |
Codecov Report
@@ Coverage Diff @@
## master #638 +/- ##
==========================================
- Coverage 88.83% 88.81% -0.02%
==========================================
Files 92 92
Lines 11278 11278
Branches 1848 1848
==========================================
- Hits 10019 10017 -2
- Misses 925 926 +1
- Partials 334 335 +1
Continue to review full report at Codecov.
|
Float64 array images return the array from get_fdata, with default input arguements, but other data types return the array cast to float64. Tests will fail before merge of fix in nipy#638 .
|
@matthew-brett This LGTM. Did you want to get this and #637 in before the 2.3.0 release? |
|
Yes, please - I would love to get these in before the release. |
We document that get_fdata returns the contained array, from an array image, if
it is of the desired type, but in fact we were always making a copy.
This revealed from updating the doctests in the documentation.
Refactor the image API tests to be a bit more readable (a tiny bit), fix the
bug, and add tests.