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

[DOC] Add note about decreasing memory usage #2223

Merged
merged 2 commits into from
Nov 21, 2019
Merged

[DOC] Add note about decreasing memory usage #2223

merged 2 commits into from
Nov 21, 2019

Conversation

dPys
Copy link
Contributor

@dPys dPys commented Nov 20, 2019

...by decompressing nii's per issue #2218 at @GaelVaroquaux 's request.

@GaelVaroquaux GaelVaroquaux changed the title [DOC][skip ci] Add note about decreasing memory usage [MRG+1][DOC] Add note about decreasing memory usage Nov 20, 2019
@GaelVaroquaux
Copy link
Member

LGTM. Thank you very much. It's a pity that you skipped CI, as we would have looked at the docs rendered by the CI.

Another review by someone else?

@kchawla-pi
Copy link
Collaborator

kchawla-pi commented Nov 20, 2019 via email

@dPys dPys changed the title [MRG+1][DOC] Add note about decreasing memory usage [DOC] Add note about decreasing memory usage Nov 20, 2019
@kchawla-pi
Copy link
Collaborator

kchawla-pi commented Nov 20, 2019

I can't force build it directly, however there is a workaround.
Merging this PR into the upstream test-circleci branch without the commit log and pushing it will build it.
You can check i tout here once it is done:
https://circleci.com/gh/kchawla-pi/nilearn/644

@kchawla-pi
Copy link
Collaborator

Thanks for the force push @dPys

@dPys
Copy link
Contributor Author

dPys commented Nov 20, 2019

Already one step ahead of you @kchawla-pi !

I had some further text I wanted to add so this may have turned out to be a blessing in disguise.

@codecov
Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #2223 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2223   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         149      149           
  Lines       18991    18991           
  Branches     2307     2307           
=======================================
  Hits        17541    17541           
  Misses        931      931           
  Partials      519      519

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 0941897...bf80743. Read the comment docs.

.. topic:: Decreasing memory used when loading Nifti images

When Nifti images are stored compressed (.nii.gz), loading them directly
consumes more memory. As a result, large 4D images may
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth explaining why this is true ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My sense is that it may not be worth getting too much into the weeds about why. @GaelVaroquaux ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Less is more

Co-Authored-By: Elizabeth DuPre <emd222@cornell.edu>
@dPys
Copy link
Contributor Author

dPys commented Nov 21, 2019

@GaelVaroquaux -- one final question from me on this:

why not run:

def make_uncompressed_temp(func_file):
    if func_file.endswith('.nii.gz'):
        import tempfile
        import gzip
        import os
        temp_path = "{}/{}.nii".format(tempfile.mkdtemp(), os.path.basename(func_file))
        decomp_func = gzip.open(path)
        with open(temp_path, 'wb') as outfile:
            outfile.write(decomp_func.read())
        decomp_func.close()
        outfile.close()
    else:
        temp_path = func_file
    return temp_path

every time as a default precursor to all Nilearn functions, subsequently deleting the temporary file after operations are performed? Wouldn't that reduce the memory burden universally?

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Nov 21, 2019 via email

Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@GaelVaroquaux
Copy link
Member

The docs can be viewed here: https://5013-1235740-gh.circle-artifacts.com/0/home/circleci/project/doc/_build/html/manipulating_images/input_output.html#niimg-like-objects

They look good. I am merging. I just realized that I'd like this title to be bold, but I'll fix it after merge.

Thanks a lot!

@GaelVaroquaux GaelVaroquaux merged commit b76551f into nilearn:master Nov 21, 2019
@dPys
Copy link
Contributor Author

dPys commented Nov 21, 2019

Many reasons why this would be fragile: - It requires a system to clean up the file after, and these are challenging to make fool proof (think: crashing in the middle of reading) - It will slow down the loading (though not much) - It will take disk space, and sometimes there is none available (many processes loading in the same time), or mkdtemp might not return the right place (shared systems not well configured) However, what this gets to is the tradeoff between memory usage and disk usage, which is a classic one. The right way to do this tradeoff would be to increase the swap. Unfortunately, in linux the swap cannot be easily increased dynamically.

Yep, well a useful hack anyway even if it doesn't scale :-) I did think about addressing this problem with swap but without being able to modify swappiness nor an interest in getting into the weeds with it, eventually just decided to try tmp.

Anyway, glad to see this PR merged.

Cheers

@dPys dPys deleted the doc/dec_mem_nii_imgs branch November 21, 2019 06:27
kchawla-pi added a commit to kchawla-pi/nilearn that referenced this pull request Nov 21, 2019
* 'master' of github.com:nilearn/nilearn:
  Add testing for Nilearn setup & install & Fix the broken dependency installation (nilearn#2201)
  Fix uniform ball cloud test for sklearn >= 0.22 (nilearn#2175)
  MAINT: sklearn Deprecations (nilearn#2219)
  DOC: title in bold
  [DOC] Add note about decreasing memory usage (nilearn#2223)
kchawla-pi added a commit to pausz/nilearn that referenced this pull request Nov 21, 2019
…smooth-image

* 'master' of https://github.com/nilearn/nilearn: (25 commits)
  Add testing for Nilearn setup & install & Fix the broken dependency installation (nilearn#2201)
  Fix uniform ball cloud test for sklearn >= 0.22 (nilearn#2175)
  MAINT: sklearn Deprecations (nilearn#2219)
  DOC: title in bold
  [DOC] Add note about decreasing memory usage (nilearn#2223)
  Rel 060b0 (nilearn#2208)
  Nilearn 0.6.0b0 release (nilearn#2206)
  Fixed the redundant & missing test case in merged PR nilearn#2035 (nilearn#2205)
  Modify fetch_development_fmri to fetch adults or children (nilearn#2035)
  Verbose doc building to ease tracking of progress & diagnose stalls (nilearn#2203)
  New conda env is created once conda path has been activated
  Conda environment is created for full-builds
  Refactor CircleCI config for reduced redundancy (nilearn#2204)
  Installation should fail on Python < 3.5 (nilearn#2198)
  [MRG] Add get data function (nilearn#2172)
  fix error when colorscale given boolean array (nilearn#2193)
  remove is_valid filter (nilearn#2169)
  Moved new entries to next release
  FIX: marker size issue in plot_connectome nilearn#2185 (nilearn#2186)
  Updated Appveyor status badge
  ...
kchawla-pi added a commit to darya-chyzhyk/nilearn that referenced this pull request Nov 24, 2019
…s-craddock

* 'master' of https://github.com/nilearn/nilearn: (620 commits)
  CI: Test pre-release versions of dependencies (nilearn#2224)
  Fix:  nilearn.image.smooth_img fails if fwhm is an ndarray (nilearn#2107)
  Update Python version warnings for Py35 deprecation (nilearn#2214)
  Fixing failure in minimum build of TravisCI (nilearn#2227)
  Updated missing whats_new entry
  Add testing for Nilearn setup & install & Fix the broken dependency installation (nilearn#2201)
  Fix uniform ball cloud test for sklearn >= 0.22 (nilearn#2175)
  MAINT: sklearn Deprecations (nilearn#2219)
  DOC: title in bold
  [DOC] Add note about decreasing memory usage (nilearn#2223)
  Rel 060b0 (nilearn#2208)
  Nilearn 0.6.0b0 release (nilearn#2206)
  Fixed the redundant & missing test case in merged PR nilearn#2035 (nilearn#2205)
  Modify fetch_development_fmri to fetch adults or children (nilearn#2035)
  Verbose doc building to ease tracking of progress & diagnose stalls (nilearn#2203)
  New conda env is created once conda path has been activated
  Conda environment is created for full-builds
  Refactor CircleCI config for reduced redundancy (nilearn#2204)
  Installation should fail on Python < 3.5 (nilearn#2198)
  [MRG] Add get data function (nilearn#2172)
  ...

# Conflicts:
#	nilearn/datasets/atlas.py
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.

None yet

4 participants