Add verification of out-of-bound with map_coordinates #1270
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1270 +/- ##
=======================================
Coverage 72.45% 72.46%
=======================================
Files 293 293
Lines 25203 25218 +15
Branches 3530 3532 +2
=======================================
+ Hits 18262 18273 +11
Misses 5450 5450
- Partials 1491 1495 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| import nibabel as nib | ||
| import numpy as np | ||
| import matplotlib.pyplot as plt | ||
| from scipy.ndimage import map_coordinates |
| import nibabel as nib | ||
| from nilearn import plotting | ||
| import numpy as np | ||
| from scipy.ndimage import map_coordinates |
AntoineTheb
left a comment
AntoineTheb
left a comment
There was a problem hiding this comment.
See comment. I would strongly recommend we actually deal with out of bounds coordinates in the function. Otherwise, we'll redo the same logic every time we call our own map_coordinates.
| if (np.any(np.logical_or(points[0] < 0, points[0] > data.shape[0])) or | ||
| np.any(np.logical_or(points[1] < 0, points[1] > data.shape[1])) or | ||
| np.any(np.logical_or(points[2] < 0,points[2] > data.shape[2]))) : | ||
| logging.warning("Careful! You are interpolating outside of boundaries " |
There was a problem hiding this comment.
I don't like this as it deviates from the expected behavior of the function. I would instead try to match the expected behavior of map_coordinates and return a cval if the indices are actually out of bounds instead of logging a message (which may be swallowed).
In this case, we would expect "E" to return an "out-of-bounds" value that the user can choose, not what is in the nearest voxel. |
3 participants
In PR #1102, we changes the padding to nearest. Antoine suggested to add a verification for out-of-bound values. It was not done in PR1102 to keep it simple, but was added as issue #1206. This finishes PR1102.