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] Adding support for on-the-fly transforms #198

Merged
merged 2 commits into from
Apr 14, 2021

Conversation

hubertjb
Copy link
Collaborator

@hubertjb hubertjb commented Apr 8, 2021

This PR adds support for on-the-fly transforms in BaseDataset and WindowsDataset. It also adds a similar API to modify the underlying transforms in BaseConcatDataset.

It is based on the way transforms are implemented in torchvision: https://pytorch.org/tutorials/beginner/data_loading_tutorial.html

@hubertjb
Copy link
Collaborator Author

It was brought to my attention that the discussion on #177 leaned toward using the collate_fn argument of DataLoader for implementing transforms as it allows batch-level augmentations.

With this in mind, do we want to also include a simpler Dataset-based implementation as in torch-vision and torch-audio? That's what @robintibor suggested here: #177 (comment)

@cedricrommel

@robintibor
Copy link
Contributor

Sorry @sbbrandt I meant to ask here haha:
You also had something in this direction? How do you feel about this implementation? Is it compatible with what you had?

@robintibor
Copy link
Contributor

But based on extensive previous discussion (thanks for linking @hubertjb ) I would also say this way could be merged already. and then batch level we can add separately at a later point.

@sbbrandt
Copy link
Collaborator

Sorry @sbbrandt I meant to ask here haha:
You also had something in this direction? How do you feel about this implementation? Is it compatible with what you had?

@robintibor Yes, makes more sense ;)

This implementation is also nice and works for many DA methods. E.g. mixup would need another implementation, but can still be implemented via a custom DataLoader, as well as betch level manipulations.

@robintibor
Copy link
Contributor

Can you rebase this on current master to see if CIs run @hubertjb ?

For example in my setup steps would be:

git checkout master
git pull upstream master
git checkout on-the-fly-transforms
git rebase master
git push origin on-the-fly-transforms --force

@hubertjb hubertjb changed the title [WIP] Adding support for on-the-fly transforms [MRG] Adding support for on-the-fly transforms Apr 13, 2021
…aseConcatDataset to enable on-the-fly transforms

- updating docstrings
- adding test
Copy link
Contributor

@robintibor robintibor left a comment

Choose a reason for hiding this comment

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

Just would like to know answer, then we can merge

braindecode/datasets/base.py Outdated Show resolved Hide resolved
@robintibor
Copy link
Contributor

Ok I applied the changes then, also because I felt curious how Github UI works for this hihi @hubertjb

@robintibor
Copy link
Contributor

Ok great, Github Actions succeeded, so merging! :) Thanks for the work

@robintibor robintibor merged commit 4777463 into braindecode:master Apr 14, 2021
@hubertjb hubertjb deleted the on-the-fly-transforms branch April 14, 2021 23:45
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