Skip to content

ENH: Remove img.get_data() from internal use #809

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 27, 2019
Merged

ENH: Remove img.get_data() from internal use #809

merged 3 commits into from
Sep 27, 2019

Conversation

effigies
Copy link
Member

@effigies effigies commented Sep 15, 2019 •
edited
Loading

Follow-up to #794. This removes almost all calls to get_data() from nibabel internals.

The exception is nibabel.nicom.dicomwrappers.*.get_data, where get_fdata() hasn't been implemented (see #797).

Begins by adding a regression test to reproduce #802.

Fixes #802.

@effigies effigies added this to the 3.0.0 RC1 milestone Sep 15, 2019
@effigies effigies force-pushed the enh/purge_internal_get_data branch from 83ff248 to 20df4c8 Compare September 15, 2019 17:39
@codecov
Copy link

codecov bot commented Sep 15, 2019 •
edited
Loading

Codecov Report

Merging #809 into master will decrease coverage by <.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #809      +/-   ##
=========================================
- Coverage    90.1%   90.1%   -0.01%     
=========================================
  Files          96      96              
  Lines       11907   11906       -1     
  Branches     2124    2124              
=========================================
- Hits        10729   10728       -1     
  Misses        833     833              
  Partials      345     345
Impacted Files Coverage Δ
nibabel/__init__.py 92.85% <ø> (ø) ⬆️
nibabel/minc2.py 90% <ø> (ø) ⬆️
nibabel/brikhead.py 97.45% <ø> (ø) ⬆️
nibabel/spaces.py 90% <ø> (ø) ⬆️
nibabel/loadsave.py 90.38% <ø> (ø) ⬆️
nibabel/dataobj_images.py 95.08% <ø> (ø) ⬆️
nibabel/filebasedimages.py 87.41% <ø> (ø) ⬆️
nibabel/cmdline/ls.py 16.45% <0%> (ø) ⬆️
nibabel/freesurfer/mghformat.py 95.45% <100%> (ø) ⬆️
nibabel/processing.py 100% <100%> (ø) ⬆️
... and 4 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 b46efa3...783a0d0. Read the comment docs.

@effigies effigies force-pushed the enh/purge_internal_get_data branch 4 times, most recently from 9a712dd to a8d304a Compare September 15, 2019 20:33
@effigies effigies force-pushed the enh/purge_internal_get_data branch from a8d304a to c8d93e6 Compare September 15, 2019 22:59
@effigies effigies changed the title ENH: Remove img.get_data() from internal use except for appropriate tests ENH: Remove img.get_data() from internal use Sep 15, 2019
@effigies
Copy link
Member Author

This is ready for a review. It's a lot of individual choices between np.asanyarray(img.dataobj) and img.get_fdata(), and a couple doc updates.

As a rule, I went with the array(dataobj) approach for internal manipulations, where there are potential memory savings, and the resulting data array is not directly exposed to the user.

Tended to move to get_fdata() in tests, unless it seemed to be directly testing the variable scaling.

matthew-brett
matthew-brett reviewed Sep 16, 2019
View reviewed changes
Copy link
Member

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

Nice - thanks - I would have made the same decisions about where to go for np.asanyarray(img.dataobj) .

nibabel/tests/test_image_api.py Outdated
cache. Implement this as a no-op if ``get_fdata()``, ``get_data`` do not
* ``img.uncache()`` (``img.get_fdata()`` and ``img.get_data()`` (deprecated) are
allowed to cache the result of the array creation. If they do, this call empties
that cache. Implement this as a no-op if ``get_fdata()``, ``get_data`` do not
Copy link
Member

Choose a reason for hiding this comment

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

Oops , missing parens in original, should be get_data() .

nibabel/tests/test_image_api.py Outdated
* ``img.uncache()`` (``img.get_data()`` and ``img.get_data`` are allowed to
cache the result of the array creation. If they do, this call empties that
cache. Implement this as a no-op if ``get_fdata()``, ``get_data`` do not
* ``img.uncache()`` (``img.get_fdata()`` and ``img.get_data()`` (deprecated) are
Copy link
Member

Choose a reason for hiding this comment

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

Can you think of phrasing that cannot imply get_fdata is also deprecated?

nibabel/tests/test_image_api.py
@@ -150,19 +147,28 @@ def validate_filenames(self, imaker, params):
# to_ / from_ filename
fname = 'another_image' + self.standard_extension
with InTemporaryDirectory():
img.to_filename(fname)
rt_img = img.__class__.from_filename(fname)
# Validate that saving or loading a file doesn't use deprecated methods internally
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@effigies
Copy link
Member Author

@matthew-brett Docs updated. Any last comments?

@matthew-brett
Copy link
Member

Thanks - all good.

@effigies
Copy link
Member Author

Cool. Thanks for the review.

@effigies effigies merged commit 370b50f into nipy:master Sep 27, 2019
@effigies effigies modified the milestones: 3.0.0 RC1, 3.0.0 Oct 23, 2019
@effigies effigies deleted the enh/purge_internal_get_data branch October 23, 2019 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthew-brett matthew-brett matthew-brett left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

BUG: Simple save-load round-trip emits warning
2 participants
@effigies @matthew-brett