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

Implementation for learning curves #155

Merged
merged 18 commits into from
Apr 6, 2021

Conversation

jsosulski
Copy link
Collaborator

Commit log was getting a mess with all the reformatting. This is the continuation of #132. Fixes #129

I would prefer to submit another PR regarding the "samples_per_class" feature, as this requires a bit more debugging and an additional example. Or do you prefer one large PR?

Also: added Makefile for quick venv setup

Added external examples, which require additional dependencies

Squashed commit of the following:

commit e4b3eb97b6414be8a195e411551927286dbe4260
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Wed Mar 17 16:05:49 2021 +0100

    split examples

commit 2c0c031a33583df088f1bbf8195574b128ba01ac
Merge: 6e9eef8 4161d08
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Wed Mar 17 09:47:07 2021 +0100

    Merge branch 'master' into creating_learning_curves

commit 6e9eef8
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Tue Feb 16 18:19:25 2021 +0100

    cleanup, some rough flake8

commit ceb7e54
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Wed Feb 10 17:21:26 2021 +0100

    add reproducibility + timing

commit b7a8fbd
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Fri Feb 5 19:02:58 2021 +0100

    Initial working solution

commit bef0dcc
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Fri Feb 5 18:45:28 2021 +0100

    add makefile for easier setup and teardown

commit 14fadf4
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Sun Dec 20 13:32:46 2020 +0100

    remove superfluous print statement

commit a2885c8
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Sun Dec 20 13:26:19 2020 +0100

    flake8

commit 26de77e
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Sun Dec 20 13:23:30 2020 +0100

    Add initial unit test for additional columns feature

commit 50cd2ae
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Sun Dec 20 13:21:26 2020 +0100

    Check if specified columns are also returned in the evaluation

commit e0db743
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Sun Dec 20 12:42:13 2020 +0100

    Allow to specify additional columns for evaluation

    This can be used in custom evaluations

commit 2a50e7b
Author: V G <vladislav.goncharenko@phystech.edu>
Date:   Thu Jan 21 16:29:17 2021 +0300

    deleted outdated initialization

commit ce90a66
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Fri Dec 11 16:56:46 2020 +0100

    fix flake8 errors

commit 660964e
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Fri Dec 11 14:28:07 2020 +0100

    fix P300 fake datasets in unit tests

commit aa14f9c
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Tue Jun 30 18:07:57 2020 +0200

    merge two if-lines + compare type equality with is instead of ==

commit 2d086ad
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Thu Feb 6 16:52:00 2020 +0100

    allow event lists in P300 paradigm

commit e34e3d3
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Mon Feb 1 09:10:57 2021 +0100

    comments from meeting

commit f6c4207
Merge: cb19cc5 009d6fa
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Thu Jan 14 13:18:06 2021 +0100

    Merge branch 'master' into creating_learning_curves

commit cb19cc5
Author: Jan Sosulski <mail@jan-sosulski.de>
Date:   Wed Jan 13 16:43:23 2021 +0100

    initial implementation for learning curve creation
@jsosulski
Copy link
Collaborator Author

BTW, CI does not like 6 nested for loops :-) gotta check tomorrow how to fix this.

@jsosulski
Copy link
Collaborator Author

Oh btw, sorry I added a Makefile in this PR, I use it alot for a quick setup / teardown of my venv and running all tests etc.

@jsosulski
Copy link
Collaborator Author

jsosulski commented Mar 18, 2021

Changed my mind and added per_class functionality to this PR. Also added an example script for motor imagery learning curve that (used to) produce the following plot. Note that number of samples = number of samples of all classes, i.e. for a two-class problem with 5 samples per class we have 10 training samples.

grafik

@jsosulski
Copy link
Collaborator Author

Should be ready for review.

@sylvchev sylvchev self-requested a review March 18, 2021 17:38
Copy link
Collaborator Author

@jsosulski jsosulski left a comment

Choose a reason for hiding this comment

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

Remove Makefile from PR

Copy link
Member

@sylvchev sylvchev left a comment

Choose a reason for hiding this comment

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

This new PR is really nice and the examples are awesome. Thank you so much to show 2 different setup, for both policies ('ratio' and 'per_class') in 2 different paradigms!
I have some question regarding the value of n_perm, but the PR could be merge quickly!

examples/plot_learning_curve_motor_imagery.py Outdated Show resolved Hide resolved
examples/plot_learning_curve_p300.py Outdated Show resolved Hide resolved
examples/plot_within_session_p300.py Show resolved Hide resolved
examples/plot_within_session_p300.py Show resolved Hide resolved
examples/external/README.md Outdated Show resolved Hide resolved
moabb/evaluations/evaluations.py Outdated Show resolved Hide resolved
moabb/evaluations/evaluations.py Outdated Show resolved Hide resolved
moabb/evaluations/evaluations.py Show resolved Hide resolved
@sylvchev
Copy link
Member

@v-goncharenko As you have guess it, the external examples are for examples with extra-dependencies, that are not (yet!) part of the core MOABB dependencies. Do you have some recommendations regarding how to handle these extra dependencies with poetry?

@v-goncharenko
Copy link
Collaborator

v-goncharenko commented Mar 24, 2021

@v-goncharenko As you have guess it, the external examples are for examples with extra-dependencies, that are not (yet!) part of the core MOABB dependencies. Do you have some recommendations regarding how to handle these extra dependencies with poetry?

Yes, that could be done easily with poetry add --optional <git repo address> (see docs)
Then one need to add this dependency to "extras".
If you haven't done this at least once it could be harder. But I think we will make some current dependencies optional shortly and having an example it will be really easy.

In this case either @jsosulski could add it (see link above) or if we move this branch to main repo I could make a commit updating dependencies.
Additionally both cases require rebasing to current master.

initialized n_perms as None to make it explicit that
classic WithinSessionEvaluation is the default

check that n_perms is monotonically decreasing

Author / description changes in the examples
@v-goncharenko
Copy link
Collaborator

I guess it's better to rebase rather merge this changes...
Merging can cause unexpected and undesired changes in code that was edited twice =(

@jsosulski
Copy link
Collaborator Author

Sure, I am fine with a rebase, but before that, is there a better way to get py3.6 on a kubuntu 20.04 installation without installing a new repository to my system?

@jsosulski
Copy link
Collaborator Author

Ah, I again had the issue that isort and black result in different sorting and manually ran them in the wrong order. But now I have a py3.6 setup with pre-commit running so the latest commit should fix the issues.

@jsosulski
Copy link
Collaborator Author

After running poetry add --optional git+https://github.com/jsosulski/tdlda.git#0.1.0 do I commit both poetry.lock and pyproject.toml?

Installing 'external' dependencies allows one to run external examples

Modified Readme accordingly
@jsosulski jsosulski requested a review from sylvchev March 24, 2021 18:00
@sylvchev sylvchev closed this Mar 25, 2021
@sylvchev sylvchev reopened this Mar 25, 2021
@sylvchev
Copy link
Member

I just corrected the comment indicating the author (which was messed up with isort I think) and the pipeline description in the header, that were not updated.

Good to merge!

@jsosulski jsosulski requested a review from sylvchev March 26, 2021 09:49
@sylvchev
Copy link
Member

For later, we could add automatic determination of permutation to ensure a statistical significance. I do not have any formula yet, so we could leave it the next update.

@sylvchev sylvchev added this to the 0.4.0 milestone Mar 30, 2021
@sylvchev sylvchev merged commit d0ddf2e into NeuroTechX:master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow creation of learning curves
3 participants