ENH add dtype option for NiftiMasker

[SylvainLan]

This PR aims at solving issue #1669

I only used _utils.check_niimg which already has this dtype feature and is used in both NiftiMasker and MultiNiftiMasker

[GaelVaroquaux]

You're on fire!!! That's awesome!!

I don't promise that we merge this PR before the release, but let's see.

[GaelVaroquaux]

Any chance that I can convince you to add this for the other maskers in input_data? Let's not do it in things like CanICA for now, as we'll need , but the other maskers would need it for consistency.

And maybe a note (directive ".. topic:: ") in the documentation, for instance on http://nilearn.github.io/manipulating_images/masker_objects.html#common-data-preparation-steps-smoothing-filtering-resampling
That note should say that forcing float32 can help save memory and is often a good-enough numerical precision.

Also, this needs an entry in whats_new.

[SylvainLan]

Any chance that I can convince you to add this for the other maskers in input_data

Sure

And maybe a note (directive ".. topic:: ") in the documentation

OK !

[bthirion]

A test would be welcome ! LGTm otherwise.

[GaelVaroquaux]

Could you rebase or merge on master, to get the tests to pass?

[GaelVaroquaux]

This is great!

We still need the added paragraph to the discussion, as mentioned in my above review, as well as as an entry in the whats_new.rst file. Something important is that the entry in the whats_new should state that there is a change in default behavior: now nifti images will more often get cast to float32.

[SylvainLan]

We still need the added paragraph to the discussion, as mentioned in my above review, as well as as an entry in the whats_new.rst file. Something important is that the entry in the whats_new should state that there is a change in default behavior: now nifti images will more often get cast to float32

I'm doing it ! But the default dtype option right now is None, which won't change the dtype, so either we can make default to auto or leave it to None. What do you think ?

[codecov]

Codecov Report

Merging #1711 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1711      +/-   ##
==========================================
+ Coverage   94.86%   94.88%   +0.02%     
==========================================
  Files         134      134              
  Lines       16925    16954      +29     
==========================================
+ Hits        16056    16087      +31     
+ Misses        869      867       -2

Impacted Files	Coverage Δ
nilearn/input_data/nifti_masker.py	96.1% <100%> (+0.05%)	⬆️
...ilearn/input_data/tests/test_multi_nifti_masker.py	100% <100%> (ø)	⬆️
nilearn/input_data/nifti_labels_masker.py	97.67% <100%> (+0.02%)	⬆️
nilearn/input_data/nifti_maps_masker.py	96.39% <100%> (+0.03%)	⬆️
nilearn/regions/signal_extraction.py	96.82% <100%> (ø)	⬆️
nilearn/input_data/multi_nifti_masker.py	96.1% <100%> (+0.05%)	⬆️
nilearn/input_data/tests/test_nifti_masker.py	100% <100%> (ø)	⬆️
nilearn/input_data/base_masker.py	85.5% <100%> (ø)	⬆️
nilearn/input_data/nifti_spheres_masker.py	95.76% <100%> (+0.07%)	⬆️
nilearn/signal.py	91.12% <0%> (+1.18%)	⬆️

---

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 fcb4efd...8250eb5. Read the comment docs.

[SylvainLan]

This PR is open for reviews !

[GaelVaroquaux]

LGTM. +1 for merge when CI is green.

[GaelVaroquaux]

Merging!