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

Q: decoding module sufficiently stable for 0.13? #3592

Closed
kingjr opened this issue Sep 15, 2016 · 14 comments
Closed

Q: decoding module sufficiently stable for 0.13? #3592

kingjr opened this issue Sep 15, 2016 · 14 comments

Comments

@kingjr
Copy link
Member

kingjr commented Sep 15, 2016

The next release is coming soon, and I am wondering whether the recent enh in the decoding module are sufficiently stable for it.

We've encountered very painful refactoring in the past because of lack of experience, and we should clearly avoid this mistake again.

I am most worried about :

  1. the location of XdawnTransformer currently in mne.preprocessing, although may need to be moved to mne.decoding

  2. the SearchLight and GeneralizationLight: although working & stable, these classes currently add little from the user's perspective, as they do, with a cleaner code, the same as TimeDecoding and GeneralizationAcrossTime. Also the name 'GeneralizationLight` may not be appropriate.

I reckon we should put them back to private until 0.14 to give us some distance. WDYT?

@dengemann
Copy link
Member

If you don't feel a strong rush. We can let it mature until christmas. In
the case of doubt slowing down is good. But I have no strong emotions here.

On Thu, Sep 15, 2016 at 12:20 PM Jean-Rémi KING [email protected]
wrote:

The next release is coming soon, and I am wondering whether the recent enh
in the decoding module are sufficiently stable for it.

We've encountered very painful refactoring in the past because of lack of
experience, and we should clearly avoid this mistake again.

I am most worried about :

  1. the location of XdawnTransformer currently in mne.preprocessing,
    although may need to be moved to mne.decoding

  2. the SearchLight and GeneralizationLight: although working & stable,
    these classes currently add little from the user's perspective, as they do,
    with a cleaner code, the same as TimeDecoding and GeneralizationAcrossTime.
    Also the name 'GeneralizationLight` may not be appropriate.

I reckon we should put them back to private until 0.14 to give us some
distance. WDYT?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3592, or mute the thread
https://github.com/notifications/unsubscribe-auth/AB0fit_WpPbg9WNUB4-NTUYIxbEwzKdIks5qqRwIgaJpZM4J9tbr
.

@larsoner
Copy link
Member

larsoner commented Sep 15, 2016 via email

@agramfort
Copy link
Member

agramfort commented Sep 15, 2016 via email

@kingjr
Copy link
Member Author

kingjr commented Sep 15, 2016

The decoding was a complete mess where each object had its own API; we've been trying to clean it up, but this task isn't finished IMO. I think releasing a half baked module isn't worth the deprecation cycles that we need support once an object is in stable.

@agramfort
Copy link
Member

agramfort commented Sep 15, 2016 via email

@kingjr
Copy link
Member Author

kingjr commented Sep 15, 2016

I'm trying to keep the work plan in #3442, but it tends to grow a bit at each new PR.

@kingjr
Copy link
Member Author

kingjr commented Sep 22, 2016

@agramfort ok to temporally privatize XdawnTransformer, SearchLight and GeneralizationLight before the release? The underlying functions are still covered by Xdawn, TimeDecoding and GeneralizationTime.

@agramfort
Copy link
Member

agramfort commented Sep 23, 2016 via email

@kingjr
Copy link
Member Author

kingjr commented Sep 23, 2016

As mentioned earlier,

  • XdawnTransformer should probably be in decoding not in preprocessing
  • GeneralizationLight is probably not a good name.
  • All these objects are largely redundant with the previous equivalents (Xdawn, TimeDecoding, GeneralizationAcrossTime), but the latter don't get deprecated yet; which thus create multiple ways of doing the same thing.
  • All of these objects are 3 months old, no one use them but me.

The annoyance of revising objects is much larger than the gain of presenting them early / at Biomag. I don't understand your reluctance to delay their public release?

@agramfort
Copy link
Member

agramfort commented Sep 24, 2016 via email

@kingjr
Copy link
Member Author

kingjr commented Sep 25, 2016

ok, I'll wait for asish to clean up his PR and do that.

Also, thinking about it SearchLight may also not be the appropriate term. What we do seems closer to a slicing.

FWIU slicing consists in analyzing a subset of the hyperdimensional space (e.g. all sensors at a given time points), whereas the fMRI SearchLight is used with redundant and overlapping subsets (e.g. voxel 1-10, then voxel 2-11, etc).

@agramfort
Copy link
Member

agramfort commented Sep 25, 2016 via email

@kingjr
Copy link
Member Author

kingjr commented Sep 25, 2016

Ok, I'll take care of this within the next 48hours

@kingjr
Copy link
Member Author

kingjr commented Sep 26, 2016

closed via #3617

@kingjr kingjr closed this as completed Sep 26, 2016
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 a pull request may close this issue.

4 participants