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

[MRG] TimeDistributed class to apply a feature extractor model on a sequence of input windows #318

Merged
merged 4 commits into from
Sep 9, 2021

Conversation

hubertjb
Copy link
Collaborator

@hubertjb hubertjb commented Aug 6, 2021

This PR adds the TimeDistributed class that was defined in the sleep staging example to braindecode.models.util so that it can be reused elsewhere.

@hubertjb hubertjb requested a review from robintibor August 6, 2021 21:45
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #318 (d111c4d) into master (8beb041) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   80.51%   80.58%   +0.07%     
==========================================
  Files          49       49              
  Lines        3171     3183      +12     
==========================================
+ Hits         2553     2565      +12     
  Misses        618      618              

@gemeinl
Copy link
Collaborator

gemeinl commented Aug 9, 2021

test_variable_length_trials_decoding.py sometimes randomly fails. I do not know why yet. I restarted the job and all tests passed.

@robintibor
Copy link
Contributor

Great idea, let's think about naming a bit. I would suggest to move to own file, not in util.
For class name, how about PerWindowExtractor or something similar? TimeDistributed seems a little unclear to me what that could be @hubertjb

@robintibor
Copy link
Contributor

test_variable_length_trials_decoding.py sometimes randomly fails. I do not know why yet. I restarted the job and all tests passed.

Here also, rebasing now should prevent test from failing hopefully

@hubertjb
Copy link
Collaborator Author

hubertjb commented Aug 9, 2021

Great idea, let's think about naming a bit. I would suggest to move to own file, not in util.
For class name, how about PerWindowExtractor or something similar? TimeDistributed seems a little unclear to me what that could be @hubertjb

Do you think it would make more sense to move it under braindecode.models.modules?

I named it TimeDistributed because it's the term used in the paper the sleep staging example is based on, and also because it's conceptually similar to the TimeDistributed layer in keras: https://keras.io/api/layers/recurrent_layers/time_distributed/ Since it doesn't do the exact same thing I guess it might be better to use a different name as you suggest though.

I'm not sure about PerWindowExtractor because it does more than just apply a feature extractor per window. What do you think of something like SequenceAggregator or SequenceClassifier?

``n_classes`` and ``dropout``.
"""
def __init__(self, feat_extractor, feat_size=None, n_windows=None,
n_classes=None, dropout=0.25, clf=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

How about instead if clf=None, you really just return features? this would also remove immediately three arguments and make the class substantially simpler...

@robintibor
Copy link
Contributor

robintibor commented Aug 9, 2021

Yeah braindecode.models.modules would make a lot more sense I think, if not even own file, but modules could be fine. Didn't know about the keras class, but seems this one then is also different to the keras class, as the keras class really just applies the feature extractor (there called layer) right?

@hubertjb
Copy link
Collaborator Author

Maybe then it would make more sense to do something like in keras, i.e. just applying the feature extractor on multiple windows, and returning the features, and call this TimeDistributed? I'm thinking I could then move some of the logic that is specific to the sleep staging example inside braindecode.models.sleep_stager_chambon_2018.py.

@robintibor
Copy link
Contributor

yeah sounds reasonable @hubertjb

…a sequence of input windows

- adding `return_feats` argument to SleepStagerChambon2018 to return features
- updating sleep staging example with new TimeDistributed class
- adding tests
@hubertjb
Copy link
Collaborator Author

hubertjb commented Sep 6, 2021

@robintibor I simplified the implementation, now it only does the feature extraction part.

@hubertjb hubertjb changed the title [WIP] TimeDistributed class to apply a feature extractor model on a sequence of input windows [MRG] TimeDistributed class to apply a feature extractor model on a sequence of input windows Sep 6, 2021
@robintibor robintibor merged commit 83b0289 into braindecode:master Sep 9, 2021
@robintibor
Copy link
Contributor

great makes sense to me now, merged

@hubertjb hubertjb deleted the time-distributed branch September 9, 2021 18:57
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.

3 participants