Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Pacify DeprecationWarnings caused by nibabel 3 pre-release #3099

Merged
merged 15 commits into from
Dec 19, 2019

Conversation

effigies
Copy link
Member

Summary

Updated Travis on master to actually install pre-release dependencies. Starting on deprecation warnings.

I would appreciate a careful review. In general, np.[as[any]]array(niimg.dataobj) is the more conservative change, keeping get_data() functionality, except for caching. When floats obviously make more sense, I'm switching to get_fdata(). We may want to specify dtype=np.float32 in these cases.

I'm also in passing removing unnecessary loading of data blocks, such as get_data().shape or get_data().ndim.

List of changes proposed in this PR (pull-request)

  • Purge get_data()
  • ...?

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@codecov
Copy link

codecov bot commented Nov 18, 2019

Codecov Report

Merging #3099 into master will decrease coverage by <.01%.
The diff coverage is 34.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3099      +/-   ##
==========================================
- Coverage   67.79%   67.78%   -0.01%     
==========================================
  Files         295      295              
  Lines       39310    39304       -6     
  Branches     5181     5178       -3     
==========================================
- Hits        26651    26644       -7     
  Misses      11951    11951              
- Partials      708      709       +1
Flag Coverage Δ
#smoketests 51.23% <5.3%> (+0.18%) ⬆️
#unittests 65% <32.41%> (-0.04%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/dipy/preprocess.py 26.96% <0%> (ø) ⬆️
nipype/interfaces/dipy/simulate.py 22.28% <0%> (-0.45%) ⬇️
nipype/interfaces/utility/base.py 87.2% <0%> (ø) ⬆️
nipype/interfaces/nipy/preprocess.py 44.66% <0%> (-0.11%) ⬇️
nipype/interfaces/nipy/model.py 24.61% <0%> (-0.01%) ⬇️
nipype/interfaces/cmtk/parcellation.py 12.5% <0%> (ø) ⬆️
nipype/interfaces/dipy/reconstruction.py 31.68% <0%> (ø) ⬆️
nipype/algorithms/mesh.py 29.7% <0%> (ø) ⬆️
nipype/interfaces/cmtk/cmtk.py 17.96% <0%> (-0.17%) ⬇️
nipype/interfaces/dipy/tracks.py 31.28% <0%> (ø) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5cb2aa...fcbaad8. Read the comment docs.

@@ -664,7 +662,7 @@ def _run_interface(self, runtime):
)

components, filter_basis, metadata = compute_noise_components(
imgseries.get_data(),
imgseries.get_fdata(dtype=np.float32),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specifically verify that the desired behavior here would be to compute noise components on float32 data... float64 would be very large, but this data is presumably floating point, so using the dataobj interface seems needlessly cumbersome.

)

mask_images = self._process_masks(mask_images, imgseries.get_data())
mask_images = self._process_masks(mask_images, imgseries.dataobj)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the array coercion to _process_masks, since the data may not be used.

@effigies
Copy link
Member Author

This is ready for some quite tedious review. Sorry.

@effigies effigies changed the title [WIP] ENH: Pacify DeprecationWarnings caused by nibabel 3 pre-release ENH: Pacify DeprecationWarnings caused by nibabel 3 pre-release Nov 25, 2019
@effigies
Copy link
Member Author

This really should get a second pair of eyes. It's a lot of changes.

@effigies effigies added this to the 1.4.0 milestone Dec 14, 2019
Copy link
Member Author

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took another look with fresh eyes. Some proposed changes. Went ahead and cleaned up NUMPY_MMAP (see #3112) while I was at it.

I will commit the changes that are obviously needed. If someone would like to review, feel free to accept or reject the others.

maskdata = np.logical_not(np.logical_or(maskdata == 0, np.isnan(maskdata)))

session_datas = [
[
nb.load(fname, mmap=NUMPY_MMAP).get_data()[maskdata].reshape(-1, 1)
nb.load(fname, mmap=NUMPY_MMAP).get_fdata()[maskdata].reshape(-1, 1)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To control memory usage:

Suggested change
nb.load(fname, mmap=NUMPY_MMAP).get_fdata()[maskdata].reshape(-1, 1)
nb.load(fname).get_fdata(dtype=np.float32)[maskdata].reshape(-1, 1)

origdata1 = np.logical_and(
nii1.get_data() != 0, np.logical_not(np.isnan(nii1.get_data()))
)
origdata1 = np.asanyarray(nii1.dataobj)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like we're finding a mask of a volume of real values, so I would probably go with fdata. The slight potential memory advantage of dataobj doesn't justify ugliness IMO.

Suggested change
origdata1 = np.asanyarray(nii1.dataobj)
origdata1 = nii1.get_fdata(dtype=np.float32)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata1 != 0, which may not work for float.

nii1.get_data() != 0, np.logical_not(np.isnan(nii1.get_data()))
)
origdata1 = np.asanyarray(nii1.dataobj)
origdata1 = (origdata1 != 0) & ~np.isnan(origdata1)
cog_t = np.array(center_of_mass(origdata1.copy())).reshape(-1, 1)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used again, so hard to see why a copy is needed.

Suggested change
cog_t = np.array(center_of_mass(origdata1.copy())).reshape(-1, 1)
cog_t = np.array(center_of_mass(origdata1)).reshape(-1, 1)

origdata2 = np.logical_and(
nii2.get_data() != 0, np.logical_not(np.isnan(nii2.get_data()))
)
origdata2 = np.asanyarray(nii2.dataobj)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again:

Suggested change
origdata2 = np.asanyarray(nii2.dataobj)
origdata2 = nii2.get_fdata(dtype=np.float32)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata2 != 0, which may not work for float.

else:
return np.mean(min_dist_matrix)

def _eucl_max(self, nii1, nii2):
from scipy.spatial.distance import cdist

origdata1 = nii1.get_data()
origdata1 = np.asanyarray(nii1.dataobj)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
origdata1 = np.asanyarray(nii1.dataobj)
origdata1 = nii1.get_fdata(dtype=np.float32)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, now i'm confused why this function uses get_fdata, but the previous one (_eucl_mean) uses dataobj

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My criterion here was whether something appeared to be targeting integer values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roger!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata1 == 0, which may not work for float.

nipype/interfaces/dipy/simulate.py Outdated Show resolved Hide resolved
nipype/interfaces/freesurfer/tests/test_model.py Outdated Show resolved Hide resolved
nipype/interfaces/freesurfer/tests/test_model.py Outdated Show resolved Hide resolved
nipype/interfaces/nipy/preprocess.py Outdated Show resolved Hide resolved
nipype/interfaces/nipy/utils.py Outdated Show resolved Hide resolved
@effigies
Copy link
Member Author

@satra For context, the semantics are:

# Scales, but changes type only if slope/inter != 1/0
img.get_data() = np.asanyarray(img.dataobj)
img.dataobj[...] = np.asanyarray(img.dataobj)[...]  # May not load whole array into RAM

# Scales, and always sets dtype (default: float64)
img.get_fdata(dtype=dtype) = np.asanyarray(img.dataobj).astype(dtype)

# Does not scale, does not change type, may not be available for all images
img.dataobj.get_unscaled()

@effigies
Copy link
Member Author

@satra Should I interpret 👍 as "go ahead and commit" and no response as not to commit?

@satra
Copy link
Member

satra commented Dec 16, 2019

@effigies - i think it looks good to me after your semantics explanation. i have not gotten a chance to fully go through it. if you would like me to i can do it tomorrow.

@effigies
Copy link
Member Author

I would prefer if somebody fully went through it.

The suggestions I made are optional, but I generally tried to justify them. If you agree with the justifications, please merge them. If not, just resolve the conversations.

@satra
Copy link
Member

satra commented Dec 16, 2019

@effigies - i cannot merge them - i tried yesterday - it's because i don't have write access to your nipype repo.

@effigies
Copy link
Member Author

Oh weird, did they change the policy on that? Should I merge the thumbs-upped?

@effigies effigies mentioned this pull request Dec 19, 2019
11 tasks
Copy link
Member

@satra satra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks great. i left a few comments with float point comparisons. especially for those situations it seems we should convert to something where the inequality or equality holds.

does nibabel have a helper to change a float to an int that takes range into account? or we could do np.round(img).astype(np.int32)

mask = img.get_data() > 0
np.logical_or(mask, img.get_data() > 0, mask)
mask = img.get_fdata() > 0
np.logical_or(mask, img.get_fdata() > 0, mask)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a note that this may not be a good comparison for floating point data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and there are a few places in this function where it happens. it's likely not a big difference in the context of this function.

origdata2 = np.logical_not(np.logical_or(origdata2 == 0, np.isnan(origdata2)))

if isdefined(self.inputs.mask_volume):
maskdata = nb.load(self.inputs.mask_volume).get_data()
maskdata = np.asanyarray(nb.load(self.inputs.mask_volume).dataobj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is maskdata == 0, which may not work for float.

origdata1 = np.logical_not(np.logical_or(origdata1 == 0, np.isnan(origdata1)))
origdata2 = nii2.get_data()
origdata2 = np.asanyarray(nii2.dataobj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata2 == 0, which may not work for float.

else:
return np.mean(min_dist_matrix)

def _eucl_max(self, nii1, nii2):
from scipy.spatial.distance import cdist

origdata1 = nii1.get_data()
origdata1 = np.asanyarray(nii1.dataobj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata1 == 0, which may not work for float.

origdata2 = np.logical_and(
nii2.get_data() != 0, np.logical_not(np.isnan(nii2.get_data()))
)
origdata2 = np.asanyarray(nii2.dataobj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata2 != 0, which may not work for float.

origdata1 = np.logical_and(
nii1.get_data() != 0, np.logical_not(np.isnan(nii1.get_data()))
)
origdata1 = np.asanyarray(nii1.dataobj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the next line is origdata1 != 0, which may not work for float.

@effigies
Copy link
Member Author

does nibabel have a helper to change a float to an int that takes range into account? or we could do np.round(img).astype(np.int32)

Not that I know of. It's hard to think of a general solution that doesn't get so parameterized that explicitly describing what you want with numpy doesn't make more sense.

Anyway, post-3.0 the easy way will be something like: np.int32(img.dataobj), which will scale and cast and be the equivalent (but possibly memory-saving variant) of np.asanyarray(img.dataobj).astype(np.int32).

@effigies
Copy link
Member Author

@satra Thanks for the review. This is ready for merge, IMO.

@satra
Copy link
Member

satra commented Dec 19, 2019

go for it, once it clears tests.

@effigies effigies merged commit c503c32 into nipy:master Dec 19, 2019
@effigies effigies deleted the mnt/nibabel3 branch December 19, 2019 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants