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

added the rfs transforms for cifarfs #303

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brando90
Copy link

@brando90 brando90 commented Feb 5, 2022

added the rfs transforms for cifarfs

Description

Fixes #[ISSUE NUMBER]

Added the data transform according to rfs paper for cifarfs, fc100 original rfs code: https://github.com/WangYueFt/rfs/blob/f8c837ba93c62dd0ac68a2f4019c619aa86b8421/dataset/cifar.py#L26

Contribution Checklist

If your contribution modifies code in the core library (not docs, tests, or examples), please fill the following checklist.

  • My contribution is listed in CHANGELOG.md with attribution.
  • My contribution modifies code in the main library.
  • My modifications are tested.
  • My modifications are documented.

@brando90
Copy link
Author

brando90 commented Feb 5, 2022

realted PR: #305

@brando90 brando90 changed the title added the rfs transforms for cifarfs (and fc100) added the rfs transforms for cifarfs Feb 5, 2022
@seba-1511 seba-1511 mentioned this pull request Feb 8, 2022
4 tasks
Copy link
Member

@seba-1511 seba-1511 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @brando90, I've added a few comments. In addition, do you want to add your contribution to CHANGELOG.md?

mode='train',
download=True)
valid_dataset = l2l.vision.datasets.CIFARFS(root=root,
transform=data_transform,
transform=train_data_transforms,
Copy link
Member

Choose a reason for hiding this comment

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

Should be test_data_transforms, right?

@seba-1511 seba-1511 mentioned this pull request Feb 8, 2022
4 tasks
@brando90
Copy link
Author

brando90 commented Feb 8, 2022

Thanks for the PR @brando90, I've added a few comments. In addition, do you want to add your contribution to CHANGELOG.md?

More than happy to. I've not done many PRs before, so I am unsure how to do it though. Do I create another file in the PR itself or edit that file in this PR or create a new PR with the CHANGELOG.md?

@brando90
Copy link
Author

is there something missing? :)

@brando90
Copy link
Author

wouldn't it be better to have as an option for data sets to receive the transforms e.g.

def cifarfs_tasksets(
    train_ways=5,
    train_samples=10,
    test_ways=5,
    test_samples=10,
    root='~/data',
    device=None,
    **kwargs,
):

adding the option there?

@seba-1511
Copy link
Member

@brando90

is there something missing? :)

I believe the proposed changes haven't been pushed? You'd need to clone the branch, incorporate them, and push.

As for the transforms, I'd rather not include them in the benchmarks too.

@brando90
Copy link
Author

@brando90

is there something missing? :)

I believe the proposed changes haven't been pushed? You'd need to clone the branch, incorporate them, and push.

As for the transforms, I'd rather not include them in the benchmarks too.

not sure which ones you referring too but I can see the changes here: https://github.com/learnables/learn2learn/pull/303/files

@seba-1511
Copy link
Member

I meant the changes requested before merging (removing contented docs, swapping train augmentation for test, etc). As soon as these ones are pushed, we can run the tests.

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.

2 participants