[MRG] Dictionary learning + nilearn.decomposition refactoring #693

arthurmensch · 2015-07-16T15:36:20Z

Decomposition estimators (DictLearning / MultiPCA) now inherit a DecompositionEstimator.

Loading of data is made through a PCAMultiNiftiMasker, which loads data from files and compress it.

Potentially, the function check_masker could solve issue #688, as it factorizes the input checking of estimator to which you provide either a masker or parameters for a mask. It is tuned to be able to use PCAMultiNiftiMasker

bthirion · 2015-07-17T08:25:56Z

examples/connectivity/plot_dict_learning_resting_state.py

+n_components = 10
+
+dict_learning = DictLearning(n_components=n_components, smoothing_fwhm=6.,
+                             memory="nilearn_cache", memory_level=5, verbose=2, random_state=0,


memory_level=5, verbose=2, random_state=0 makes it a bit frightening to users. I think that @GaelVaroquaux already raised this issue. Couldn't we rely on defaults ? If not, this couls mean that the defaults are not well chosen...

We have to put memory_level=2 to have good performance (reuse masking from initialization in dictionary learning). I could make it the default

We have to put memory_level=2 to have good performance (reuse masking from initialization in dictionary learning). I could make it the default

memory_level=1 should always be the default. Maybe we need to change our 'func_memory_level' in a few places.

AlexandreAbraham · 2015-07-17T15:00:33Z

examples/connectivity/plot_dict_learning_resting_state.py

+=====================================================
+
+An example applying dictionary learning to resting-state data. This example applies it
+to 10 subjects of the ADHD200 datasets.


You say 10 and later you fetch 20.

AlexandreAbraham · 2015-12-03T12:04:57Z

nilearn/decomposition/base.py

+    if not hasattr(imgs, '__iter__'):
+        imgs = [imgs]
+    imgs = list(_iter_check_niimg(imgs, atleast_4d='True', memory=memory,
+                             memory_level=memory_level))


This is definitely not a good idea. You will waste a lot of memory here if you load everything instead of iterating over it.

This is definitely not a good idea. You will waste a lot of memory here if you
load everything instead of iterating over it.

My PR to this branch solves this problem, to the cost of doing things a
bit more manually. However, I think that we should accept this cost, in
order to merge this PR for the release. We should later inspect our
loading / checking code to see what is the best way to do this.

It's as simple as removing list here.

imgs = list(_iter_check_niimg(imgs, atleast_4d='True', memory=memory,

memory_level=memory_level))

It's as simple as removing list here.

No, that wasn't enough to limit the memory explosion, it just delayed it.

I suspect that images spanned by the iterator were not properly garbage
collected. But it does require investigation.

No beacuse we iterate twice. I was stupid to add this list() though. Reviewing Gaël PR now

arthurmensch · 2015-12-03T12:42:14Z

@GaelVaroquaux it should not fail if provided with images with different size isn't it ? n_voxels is based on mask_img size

GaelVaroquaux · 2015-12-03T12:43:19Z

@GaelVaroquaux it should not fail if provided with images with different size isn't it ? n_voxels is based on mask_img size

Good point, the masker does a lot to make it work.

AlexandreAbraham · 2015-12-03T13:05:00Z

Good point, the masker does a lot to make it work.

To answer you about the design flaw, I listed it in my big refactoring issue. I suggest that we merge and take a closer look afterward.

GaelVaroquaux · 2015-12-03T13:07:18Z

To answer you about the design flaw, I listed it in my big refactoring issue. I suggest that we merge and take a closer look afterward.

Absolutely!

lesteve · 2015-12-03T15:07:34Z

doc/connectivity/resting_state_networks.rst

@@ -70,11 +74,11 @@ Finally, we can plot the map for different ICA components separately:
 .. literalinclude:: ../../examples/connectivity/plot_canica_resting_state.py
    :start-after: # Finally, we plot the map for each ICA component separately

-.. |left_img| image:: ../auto_examples/connectivity/images/sphx_glr_plot_canica_resting_state_003.png
+.. |left_img| image:: ../_images/sphx_glr_plot_canica_resting_state_003.png


You probably don't want to change this link.

lesteve · 2015-12-04T10:09:03Z

Hmmm it looks like this needs a rebase on master. Maybe you'll get a few URL fixes by @mrahim for free.

arthurmensch · 2015-12-04T10:37:14Z

A rebase never comes for free ;)

lesteve · 2015-12-04T12:37:20Z

doc/connectivity/resting_state_networks.rst

+
+.. literalinclude:: ../../examples/connectivity/plot_compare_resting_state_decomposition.py
+    :start-after: # Dictionary learning
+    :end-before:# CanICA


Genuine error in CircleCI, it looks like you need a space after the :

Try to run the doc generation with make html-strict and you'll be able to reproduce the error and possibly find other problems.

GaelVaroquaux · 2015-12-04T13:20:15Z

A rebase never comes for free ;)

Quote of the week!

lesteve · 2015-12-04T14:09:58Z

doc/connectivity/resting_state_networks.rst

@@ -70,11 +74,11 @@ Finally, we can plot the map for different ICA components separately:
 .. literalinclude:: ../../examples/connectivity/plot_canica_resting_state.py
    :start-after: # Finally, we plot the map for each ICA component separately

-.. |left_img| image:: ../auto_examples/connectivity/images/sphx_glr_plot_canica_resting_state_003.png
+.. |left_img| image:: ../auto_examples/connectivity/plot_canica_resting_state_003.png


You probably shouldn't change this link

AlexandreAbraham · 2015-12-08T14:43:14Z

Merging this one. I opened issues regarding initialization documentation and broken links. Feel free to open issues if problems remain if I missed them.

…n_mask [MRG] Dictionary learning + nilearn.decomposition refactoring

GaelVaroquaux · 2015-12-08T14:45:57Z

Yipi!!!!

Sent from my phone. Please forgive brevity and mis spelling

On Dec 8, 2015, 09:44, at 09:44, Alexandre Abraham notifications@github.com wrote:

Merged #693.

Reply to this email directly or view it on GitHub:
#693 (comment)

bthirion · 2015-12-08T14:50:59Z

Congrats !

arthurmensch force-pushed the l1_dict_learning_composition_mask branch from 1f41aa0 to 40ddf78 Compare July 16, 2015 16:34

arthurmensch mentioned this pull request Jul 16, 2015

[To close] L1 dict learning composition #676

Closed

arthurmensch changed the title ~~Refactored nilearn.decomposition~~ Dict learning + nilearn.decomposition refactoring Jul 17, 2015

arthurmensch changed the title ~~Dict learning + nilearn.decomposition refactoring~~ Dictionary learning + nilearn.decomposition refactoring Jul 17, 2015

bthirion reviewed Jul 17, 2015
View reviewed changes

arthurmensch changed the title ~~Dictionary learning + nilearn.decomposition refactoring~~ [WIP] Dictionary learning + nilearn.decomposition refactoring Jul 17, 2015

arthurmensch force-pushed the l1_dict_learning_composition_mask branch from a943057 to 3e0683c Compare July 17, 2015 13:51

arthurmensch changed the title ~~[WIP] Dictionary learning + nilearn.decomposition refactoring~~ [MRG] Dictionary learning + nilearn.decomposition refactoring Jul 17, 2015

AlexandreAbraham reviewed Jul 17, 2015
View reviewed changes

AlexandreAbraham reviewed Dec 3, 2015
View reviewed changes

lesteve reviewed Dec 3, 2015
View reviewed changes

Arthur Mensch added 2 commits December 4, 2015 11:33

Added l1 code based dictionary learning

1a9e8de

Addressed comments

7af3a07

arthurmensch force-pushed the l1_dict_learning_composition_mask branch from a2c54da to 7af3a07 Compare December 4, 2015 10:36

Comments

8d15dfe

lesteve reviewed Dec 4, 2015
View reviewed changes

AlexandreAbraham mentioned this pull request Dec 8, 2015

Fix links in resting_state_networks.rst #894

Closed

AlexandreAbraham added a commit that referenced this pull request Dec 8, 2015

Merge pull request #693 from arthurmensch/l1_dict_learning_compositio…

340633d

…n_mask [MRG] Dictionary learning + nilearn.decomposition refactoring

AlexandreAbraham merged commit 340633d into nilearn:master Dec 8, 2015

lesteve mentioned this pull request Dec 14, 2015

Add dictionary learning #634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Dictionary learning + nilearn.decomposition refactoring #693

[MRG] Dictionary learning + nilearn.decomposition refactoring #693

arthurmensch commented Jul 16, 2015

bthirion Jul 17, 2015

arthurmensch Jul 17, 2015

arthurmensch Jul 17, 2015

GaelVaroquaux Jul 17, 2015 via email

AlexandreAbraham Jul 17, 2015

AlexandreAbraham Dec 3, 2015

GaelVaroquaux Dec 3, 2015

AlexandreAbraham Dec 3, 2015

GaelVaroquaux Dec 3, 2015

arthurmensch Dec 3, 2015

arthurmensch commented Dec 3, 2015

GaelVaroquaux commented Dec 3, 2015 via email

AlexandreAbraham commented Dec 3, 2015

GaelVaroquaux commented Dec 3, 2015 via email

lesteve Dec 3, 2015

lesteve commented Dec 4, 2015

arthurmensch commented Dec 4, 2015

lesteve Dec 4, 2015

GaelVaroquaux commented Dec 4, 2015 via email

lesteve Dec 4, 2015

AlexandreAbraham commented Dec 8, 2015

GaelVaroquaux commented Dec 8, 2015

bthirion commented Dec 8, 2015

[MRG] Dictionary learning + nilearn.decomposition refactoring #693

[MRG] Dictionary learning + nilearn.decomposition refactoring #693

Conversation

arthurmensch commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaelVaroquaux Jul 17, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurmensch commented Dec 3, 2015

GaelVaroquaux commented Dec 3, 2015 via email

AlexandreAbraham commented Dec 3, 2015

GaelVaroquaux commented Dec 3, 2015 via email

Choose a reason for hiding this comment

lesteve commented Dec 4, 2015

arthurmensch commented Dec 4, 2015

Choose a reason for hiding this comment

GaelVaroquaux commented Dec 4, 2015 via email

Choose a reason for hiding this comment

AlexandreAbraham commented Dec 8, 2015

GaelVaroquaux commented Dec 8, 2015

bthirion commented Dec 8, 2015