MRG: implementing / testing get_fdata #551
Conversation
Codecov Report
@@ Coverage Diff @@
## master #551 +/- ##
==========================================
+ Coverage 94.27% 94.29% +0.01%
==========================================
Files 177 177
Lines 24332 24440 +108
Branches 2610 2619 +9
==========================================
+ Hits 22940 23046 +106
- Misses 918 919 +1
- Partials 474 475 +1
Continue to review full report at Codecov.
|
| (no reference to the array). If the cache is full, "unchanged" leaves | ||
| the cache full and returns the cached array reference. | ||
|
|
||
| The cache can effect the behavior of the image, because if the cache is |
nibabel/tests/test_image_api.py
Outdated
| assert_true(is_proxy(img.dataobj)) | ||
| # Confirm it is not a numpy array | ||
| assert_false(isinstance(img.dataobj, np.ndarray)) | ||
| # Confirm it can be converted to a numpy array with is_array |
| >>> data_again[0, 0, 0, 0] | ||
| 0.0 | ||
| """ | ||
| if caching not in ('fill', 'unchanged'): |
There was a problem hiding this comment.
Want to test caching='unknown'?
| @@ -191,11 +204,155 @@ def get_data(self, caching='fill'): | |||
| self._data_cache = data | |||
| return data | |||
|
|
|||
| def get_fdata(self, caching='fill', dtype=np.float64): | |||
There was a problem hiding this comment.
any reason why float64 over float32 is the default?
There was a problem hiding this comment.
@satra - the default up till now has been to return float64 for images with scaling. Also, some images do have float64 and it seems a shame to downgrade them with this call. So float32 seems to me like an option rather than a default.
|
@matthew-brett - other than my question, which shouldn't prevent this from being merged, and on top of @effigies review, the code and documentation looks good. |
|
Shouldn't, maybe? |
|
thanks @effigies - that was definitely a lapso |
Thanks for your eagle eye.
Restore test of not-valid string specifycing caching to `get_data` and `get_fdata`.
|
Guys - thanks for the reviews. I've updated to respond to your comments, and I've added some more tests of the caching, along with a fix to the code. To help reviewing, can I suggest that y'all look at commit c729e06 separately (it has the caching changes, and tests)? |
matthew-brett
changed the title
The image cache gets reset when you ask for a new data type. For example, you might first read float64, and float64 gets cached, but then you read float32, at which point you lose the float64 cache and get a float32 cache.
matthew-brett
force-pushed
the
master
branch
from
c729e06 to
c800493
Compare
|
Oops sorry - I missed a changed I needed to make in the tests, to reflect the change in caching behavior. I rebased. New commit with all the relevant changes is c800493. |
PEP8 didn't like the indent, and my editor does, so change the syntax to make PEP8 and my editor agree.
|
+1 from me as well. |
|
@effigies - any interests in being release manager again? It's ready as far as I am concerned - but I've got 15 hours of completely new teaching to prepare in the next couple of weeks, and I'm really slammed for time. |
|
Sure. I'll aim to release by the end of the week. I'll let you know if I have any issues. |
|
Was this discussed more somewhere else? The release notes link here for information about this change, but there's no information in the PR (aside from the code itself!) that motivates the new function. (I ask as it's my vague sense that |
|
@mwaskom This thread is probably the better link... https://mail.python.org/pipermail/neuroimaging/2017-April/001383.html |
|
Thanks @effigies. Perhaps the PR intro could be edited to include this link. |
|
Done. |
get_fdata the new proposed default method for getting the data from the image.
It always returns floating point.
See also discussion at https://mail.python.org/pipermail/neuroimaging/2017-April/001383.html