Skip to content

Conversation

@sappelhoff
Copy link
Member

Picard can be used to obtain the same solution as with FastICA, Infomax, or extended Infomax --> and apparently (according to Picard 😉 ) converges faster.

This sounds like a win for me, so I added this to the docs. WDYT?

@larsoner @cbrnr @agramfort @mmagnuski @pierreablin

Info taken from README: https://github.com/pierreablin/picard

Previously discussed in gitter, Thanks @cbrnr for making me aware of it.

@cbrnr
Copy link
Contributor

cbrnr commented Jul 21, 2020

Looks good - I think this is something that people are not very much aware of. We should advertise this on the EEGLAB mailing list 😄.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

@sappelhoff
Copy link
Member Author

however, digging into this, I also saw that picard is currently not tested for Python 3 --> mind-inria/picard#22

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

Really happy to see other people saying the same thing :)

we'll catch up on testing and release / doc with @pierreablin ASAP

@sappelhoff sappelhoff changed the title DOC: note on using picard for same solution as other algos MRG, DOC: note on using picard for same solution as other algos Jul 21, 2020
@cbrnr
Copy link
Contributor

cbrnr commented Jul 21, 2020

CIs show some package conflicts (traits) - do we fix these before merging?

@GuillaumeFavelier
Copy link
Contributor

I would suggest to update with master (0b3ad03) because I don't see any -n used in .travis anymore.

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

@sappelhoff can you rebase?

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Alright, now seeing the errors described in #8040. Any ideas?

@GuillaumeFavelier
Copy link
Contributor

GuillaumeFavelier commented Jul 22, 2020

I compared with a recently merged PR and pyface has been bumped from 6.1.2 to 7.0.1. That could explain the new requirement?

5 days ago: https://travis-ci.org/github/mne-tools/mne-python/jobs/709093476#L909
this PR: https://travis-ci.org/github/mne-tools/mne-python/jobs/710681059#L906

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Probably. But which package is causing this runtime error then? There is no conda error when installing packages.

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Testing in #8042.

@GuillaumeFavelier
Copy link
Contributor

GuillaumeFavelier commented Jul 22, 2020

I think it's because pyface 7.0.1 is available on conda only recently?

Details

image

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

You mean they might have overlooked the dependency on traits? conda info pyface shows traits as a dependency without any version restriction (but maybe it should be >=6).

@GuillaumeFavelier
Copy link
Contributor

It's just a guess since it's the first big change I notice. But this could explain why there is no error during installation.

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Yes. And according to https://docs.enthought.com/pyface/changelog.html#release-7-0-1, pyface >= 7 requires traits >= 6 (which is not available in conda yet).

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

traits 6.1.0 is already in the channel (https://anaconda.org/anaconda/traits/files), so maybe this issue will resolve itself.

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Some package apparently pins traits to <6 because conda update traits says that the latest version is already installed (5.2.0).

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Maybe someone who actually knows what they are doing could help, because I don't use conda and I can't figure out what the conflict is (@hoechenberger maybe?).

@cbrnr
Copy link
Contributor

cbrnr commented Jul 22, 2020

Also, let's fix this issue in #8040 or #8042 instead of cluttering this PR.

@hoechenberger
Copy link
Member

Great move, @sappelhoff! @agramfort was waiting for somebody to step in and finally file this kind of PR ;) Good to see this happen!! We've been using Picard in my old lab and it worked really well. It's also our default now in the mne-study-template :)

Update mne/preprocessing/ica.py

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@agramfort agramfort merged commit 607fb46 into mne-tools:master Jul 22, 2020
@agramfort
Copy link
Member

thx @sappelhoff

@sappelhoff sappelhoff deleted the picard branch July 22, 2020 20:44
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.

6 participants