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

Add example of maml training on omniglot #349

Merged
merged 1 commit into from
May 27, 2021

Conversation

zou3519
Copy link
Contributor

@zou3519 zou3519 commented Apr 7, 2021

This is related to #328. This PR adds an actually correct
implementation of maml to the repo. The previous implementation doesn't
actually compute higher order gradients where it is supposed to.

I'm not familiar with how torchbench works so please let me know if
there are additional files that need to be modified.

Test Plan:

Ran the following:

python test.py -k test_maml_omniglot_example_cpu
python test.py -k test_maml_omniglot_eval_cpu
python test.py -k test_maml_omniglot_train_cpu

Future work:

  • Delete the maml example that is currently in this repo (or rename it
    to make it clear that it's doing something different from the paper that
    it is trying to reproduce).

Krovatkin
Krovatkin previously approved these changes Apr 8, 2021
Copy link
Contributor

@Krovatkin Krovatkin left a comment

Choose a reason for hiding this comment

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

:shipit: this is probably the highest density of optimizers per 10 LOCs, I've seen O_o

@zou3519
Copy link
Contributor Author

zou3519 commented May 24, 2021

@Krovatkin It's been a while since you initially reviewed this PR. I rebased on top of the master branch and the build+tests now fully succeed. Could I ask you to take another look at this please before I hit the merge button?

This is related to pytorch#328. This PR adds an actually correct
implementation of maml to the repo. The previous implementation doesn't
actually compute higher order gradients where it is supposed to.

I'm not familiar with how torchbench works so please let me know if
there are additional files that need to be modified.

Test Plan:

Ran the following:
```
python test.py -k test_maml_omniglot_example_cpu
python test.py -k test_maml_omniglot_eval_cpu
python test.py -k test_maml_omniglot_train_cpu
```

Future work:
- Delete the maml example that is currently in this repo (or rename it
to make it clear that it's doing something different from the paper that
it is trying to reproduce).
@zou3519 zou3519 requested review from xuzhao9 and Krovatkin and removed request for Chillee May 26, 2021 14:08
@xuzhao9
Copy link
Contributor

xuzhao9 commented May 27, 2021

Please help resolve the comments and I will be happy to merge your contribution!

@zou3519
Copy link
Contributor Author

zou3519 commented May 27, 2021

@xuzhao9 I resolved the comments, could you please take another look when you get a chance?

Copy link
Contributor

@xuzhao9 xuzhao9 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution!

@xuzhao9 xuzhao9 merged commit 83e54a2 into pytorch:master May 27, 2021
zou3519 added a commit to zou3519/benchmark that referenced this pull request Sep 21, 2022
Fixes pytorch#328

Here's some context:
- We discovered that the implementation doesn't actually use maml
(dragen1860/MAML-Pytorch#59)
- We filed pytorch#328
- We added maml_omniglot pytorch#349
as the correct version of of the maml model.
- We didn't delete the maml model (because I was worried that it was
doing a "different" type of maml that I hadn't seen before that is still
valid).

The last step to resolve this issue is to delete the incorrect MAML
example, unless we have reasons to keep it around.
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.

4 participants