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

Adding Sleep Models #341

Merged
merged 23 commits into from
Dec 1, 2021
Merged

Adding Sleep Models #341

merged 23 commits into from
Dec 1, 2021

Conversation

Div12345
Copy link
Contributor

Closes #334

Added the AttnSleep Model and have the example tried once here. AttnSleep requires Single-Channel EEG Data. So I used a new windows_dataset with a picked channel. I wasn't sure whether to update the existing example because then it probably won't look good with 2 different datasets creations?

@Div12345
Copy link
Contributor Author

Also primarily the code of the model is from this repo. I've put the paper reference, should I add the reference to this too somewhere?

@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #341 (fc919a3) into master (4dd51b3) will increase coverage by 0.83%.
The diff coverage is 98.47%.

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   81.90%   82.74%   +0.83%     
==========================================
  Files          51       53       +2     
  Lines        3515     3714     +199     
==========================================
+ Hits         2879     3073     +194     
- Misses        636      641       +5     

Copy link
Collaborator

@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.

@Div12345 you'll need to add test and update the documentation

ask if you need help

@robintibor
Copy link
Contributor

Yeah you could just write a simple test checking that the model forward works and has expected shapes, see https://github.com/braindecode/braindecode/blob/7f524e5f81f4caff42a1e205ae8d92281d30eeb8/test/unit_tests/models/test_models.py for inspiration, you could add a test there
Also yes, I would add the link to that repo in the docstring

@Div12345
Copy link
Contributor Author

Regarding the docs, what all should be updated? From the USleep PR I see api.rst and whats_new.rst updated. Should these just be updated manually?

Copy link
Collaborator

@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.

search for SleepStagerChambon2018 in the code base to see where the new models should be mentionned. Just add the new model names below



class AttnSleep(nn.Module):
"""Sleep Staging Architecture from Eldele et al 2021.
Copy link
Collaborator

Choose a reason for hiding this comment

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

you add 2 publics models here no?
to be consistent this could be:

SleepStagerEldele2021

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the same convention to be followed for all the sleep staging models (USleep also)? Or only because here I'm adding these together?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you add apparently 2 new public models so to me this deserves for each a doc entry, tests etc.

feel free to start with one if you prefer / find it easier.

@Div12345
Copy link
Contributor Author

I just added the tests. There is one thing with the Eldele2021, it is written very specifically for 100Hz and 30s (the convolutional layers and fully connected layers after are restricting that). There can either be a resampling at the beginning to make the overall number of samples to 3000 or I can try replacing the pooling layers with AdaptiveMax/AvgPool which provides a constant output size.

@sliwy
Copy link
Collaborator

sliwy commented Oct 25, 2021

Maybe we can use similar solution to the one used in SleepStagerChambon, so the conv/pool sizes are adjusted according to the sfreq?

time_conv_size = np.ceil(time_conv_size_s * sfreq).astype(int)
max_pool_size = np.ceil(max_pool_size_s * sfreq).astype(int)
input_size = np.ceil(input_size_s * sfreq).astype(int)
pad_size = np.ceil(pad_size_s * sfreq).astype(int)

Copy link
Collaborator

@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.

do you have some learning curves eg on physionet to see how well it learns?
you can start from the example of Chambon's model.

Comment on lines 1 to 3
# Authors: Divyesh Narayanan <divyesh.narayanan@gmail.com>
# #
# # License: BSD (3-clause)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Authors: Divyesh Narayanan <divyesh.narayanan@gmail.com>
# #
# # License: BSD (3-clause)
# Authors: Divyesh Narayanan <divyesh.narayanan@gmail.com>
#
# License: BSD (3-clause)

Comment on lines 107 to 109
"""
Forward pass.
Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""
Forward pass.
Parameters
"""Forward pass.
Parameters

Comment on lines 5 to 11
import torch
from torch import nn
import torch.nn.functional as F
import numpy as np
import math
import copy
from copy import deepcopy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import torch
from torch import nn
import torch.nn.functional as F
import numpy as np
import math
import copy
from copy import deepcopy
import math
import copy
from copy import deepcopy
import numpy as np
import torch
from torch import nn
import torch.nn.functional as F

first standard lib, then order by core component in the ecosystem (from global to local)

@@ -73,6 +73,7 @@ Enhancements
- Only load data if needed during preprocessing (e.g., allow timecrop without loading) (:gh:`164` by `Robin Tibor Schirrmeister`_)
- Adding option to sort filtered channels by frequency band for the filterbank in :func:`braindecode.datautil.filterbank` (:gh:`185` by `Lukas Gemein`_)
- Adding the USleep model :class:`braindecode.models.USleep` (:gh:`282` by `Theo Gnassounou`_ and `Omar Chehab`_)
- Adding SleepStagerEldele2021 and SleepStagerBlanco2020 models :class:`braindecode.models.SleepStagerEldele2021` and :class:`braindecode.models.SleepStagerBlanco2020` (:gh:`341` by `Divyesh Narayanan`_)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Adding SleepStagerEldele2021 and SleepStagerBlanco2020 models :class:`braindecode.models.SleepStagerEldele2021` and :class:`braindecode.models.SleepStagerBlanco2020` (:gh:`341` by `Divyesh Narayanan`_)
- Adding :class:`braindecode.models.SleepStagerEldele2021` and :class:`braindecode.models.SleepStagerBlanco2020` models for sleep staging (:gh:`341` by `Divyesh Narayanan`_)

@Div12345
Copy link
Contributor Author

@agramfort The learning curve is available here
Eldele2021 -
image

Blanco2020 -
image

The Eldele2021 learns fairly well whereas the Blanco2020 is pretty poor for this case.

@sliwy I'll check that out.

@agramfort
Copy link
Collaborator

agramfort commented Oct 25, 2021 via email

@Div12345
Copy link
Contributor Author

In my use case here it shows decent learning. But I understand, let me know if you want me to make any further specific tests to see if it's worth it or otherwise I can remove it.

@agramfort
Copy link
Collaborator

agramfort commented Oct 25, 2021 via email

@Div12345
Copy link
Contributor Author

Same Physionet, 6 class (without reduction to AASM guidelines), more data, SGD optim (vs Adam in example), data sampler to give balanced epochs.

@agramfort
Copy link
Collaborator

agramfort commented Oct 26, 2021 via email

@Div12345
Copy link
Contributor Author

I agree. I've been working on exactly that for MOABB NeuroTechX/moabb#190, there's also the corresponding issue in Braindecode #46, but I think the datasets are bigger here? So to download, save the datasets and run the DL runs, I personally don't have the resources required to do that.

@agramfort
Copy link
Collaborator

@Div12345 you mean the time or the computational ressources?

@Div12345
Copy link
Contributor Author

I mean computational resources.

@agramfort
Copy link
Collaborator

agramfort commented Oct 26, 2021 via email

@Div12345
Copy link
Contributor Author

Okay, we can continue this discussion on the Braindecode benchmarking issue, I guess then? In terms of discussing what all are to be tested out and on what datasets?

@agramfort
Copy link
Collaborator

agramfort commented Oct 26, 2021 via email

@Div12345
Copy link
Contributor Author

Div12345 commented Oct 29, 2021

@sliwy I looked into the method used in SleepStagerChambon, so the conv/pool sizes are adjusted according to the sfreq. As I understand it, though this specifically helps in the "resolution" sort of, of the CNN there, it doesn't necessarily make the final output of the same length (for example when the length of the input signal changes with sfreq same)? I think that is taken care of by the allocation of the size of the last layer using len_last_layer? Here, there are sort of 2 main submodules and the size is varying at the end of the first submodule itself.
So I tried adding a function that finds the length of the output at the end of this first submodule and using that to inform the second submodule at initialization. But this creates a problem due to a requirement where the number of attention heads in the model has to be a factor of this length. I'm not sure if making the number of attention heads in some way a dependent variable is such a good idea.

I don't know the particular usefulness of the sfreq dependent CNN sizes, but if you say it'll be useful, I can add it anyways.

@agramfort
Copy link
Collaborator

agramfort commented Oct 29, 2021 via email


# exception case for shhs to make modification in mrcnn as per implementation in the paper
shhs = False
if input_size_s == 30 and sfreq == 125 and d_model == 100:
Copy link
Collaborator

Choose a reason for hiding this comment

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

all the sleep scoring models I know work with 30s windows. I think could even enforce this. Then it's just a matter of sfreq. If sfreq==100 do this and if 125 do that. If different crash. No need to mention shhs in the code. Just put the doctsring that if sfreq==125Hz then we will use the reference architecture from XXX et al. which was validated on SHHS dataset. This is just doc, no variable name etc.

makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

but from what I understood,f or the "shhs case" we still have to perform the adaptation necessary for that case so that the model doesn't just crash (change kernel length). so we have to somehow do that right?

@agramfort
Copy link
Collaborator

agramfort commented Nov 8, 2021 via email

@robintibor
Copy link
Contributor

robintibor commented Nov 8, 2021

I mean https://github.com/braindecode/braindecode/pull/341/files#diff-4e03e96560b897b4bf4f6353fff19ca630f66347eafd96f741a62d3da843537fR197-R232

I am still not 100% sure I understood correct, but i guess we have to supply _MRCNN with the information that kernel_length=6 for that case? could also supply kernel_length=6 directly? or how do you mean? Because I understood you don't like the shhs flag passed to _MRCNN

@agramfort
Copy link
Collaborator

agramfort commented Nov 8, 2021 via email

@robintibor
Copy link
Contributor

So @Div12345 is a bit busy atm and would take a bit time before he can continue. Should we merge this in its current form or make a braindecode release without this? Otherwise from my side release could be done. What do you think @agramfort @gemeinl @hubertjb ?

@agramfort
Copy link
Collaborator

@Div12345 you need fix conflicts here

@Div12345
Copy link
Contributor Author

@agramfort I changed the name of the plot_sleep_staging example to plot_sleep_staging_chambon2018 to be more standardized like the other sleep staging examples. I think that is what is causing the conflict?

@agramfort
Copy link
Collaborator

agramfort commented Nov 20, 2021 via email

@agramfort
Copy link
Collaborator

@Div12345 you need to git merge main into this PR to fix the conflict.

@Div12345
Copy link
Contributor Author

Gives Already up to date

@agramfort
Copy link
Collaborator

agramfort commented Nov 20, 2021 via email

@Div12345
Copy link
Contributor Author

The Eldele example - Link - Just have to change the title and references a bit. The renamed Chambon - Link

@robintibor
Copy link
Contributor

I would strongly be in favour of drastically reducing nr of epochs and/or possibly dataset size to speed up example on CI. now we know it would have good performance with reasonable epochs, so we can just cut to just 1 epoch from my point of view.. right now it has 11 min running time on CI just for this example... As before, let's just be clear to the reader that this is a modified example for faster runtime and be superclear how to run it for reasonable performance...

@Div12345
Copy link
Contributor Author

@robintibor done.

@robintibor
Copy link
Contributor

so let's merge this and have a release! how do you think @agramfort ?

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
@robintibor robintibor merged commit b376d6a into braindecode:master Dec 1, 2021
@robintibor
Copy link
Contributor

Since there were no further comments, have commited the last suggestions inside and now it's merged! Thanks so much @Div12345 !! :)

@agramfort
Copy link
Collaborator

agramfort commented Dec 1, 2021 via email

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.

Adding the AttnSleep Model and another CNN model for Sleep Scoring
4 participants