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 parity test for simple RNN #1351

Merged
merged 2 commits into from
Apr 3, 2020
Merged

Conversation

mpariente
Copy link
Contributor

This is a simple test for a shifted and summed random dataset.
The test passes on CPU (I added the GPU restriction before pushing).

What does this PR do?

Compares basic lightning training to vanilla torch training for RNN.

Note

Even for the architecture for which we have performance difference between PL 0.6.0 and PL0.7.1 (as discussed in #1136), the test passes, for both versions.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Apr 2, 2020

Hello @mpariente! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-03 13:16:24 UTC

@williamFalcon
Copy link
Contributor

@mpariente so haha... where does that leave us? sounds like these parity tests show lightning is working as expected? is it maybe truncated backprop? maybe we can add it to this if it already isn't?

@mergify mergify bot requested a review from a team April 2, 2020 19:21
@mpariente
Copy link
Contributor Author

I was hoping for a little difference but even to 1e-12, it was still passing.. It's great but it's so frustrating ! You saw the tensorboard curves in #1136 right? I'm sure there's something, our code is the same, environments the same..

Anyway, this parity is just the tip of the iceberg, we don't test for any real lightning features. But this is a good start.

Has anything major changed in the callbacks behavior, or schedulers between 0.6.0 and 0.7.1?

@williamFalcon
Copy link
Contributor

Nothing big I can think of. We did automatically add adam to configure_optimizers if you don't define it. We had someone misspell that and they trained with the wrong learning rate.

Verify your learning rate?

We removed it for 0.7.2. Maybe rerun using the version from master?

Check the release notes:

https://github.com/PyTorchLightning/pytorch-lightning/releases/tag/0.7.0

@Borda Borda added feature Is an improvement or enhancement ci Continuous Integration labels Apr 2, 2020
@Borda Borda added this to the 0.7.2 milestone Apr 2, 2020
@williamFalcon
Copy link
Contributor

@mpariente yeah, those curves hint at learning rate mismatch or learning rate scheduler mismatch. maybe you guys changed the scheduler? or maybe we do something different for scheduler?

i think there was something about .step vs .epoch for the scheduler.
@jeremyjordan

something like this:
#1333

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Borda Borda requested review from jeremyjordan, MattPainter01 and a team April 2, 2020 23:19
@williamFalcon williamFalcon merged commit c51651d into Lightning-AI:master Apr 3, 2020
@Borda
Copy link
Member

Borda commented Apr 3, 2020

we forget to add it to changelog... added in a9f15df

alexeykarnachev pushed a commit to alexeykarnachev/pytorch-lightning that referenced this pull request Apr 4, 2020
* Add parity test for simple RNN

* Update test_rnn_parity.py

Co-authored-by: William Falcon <waf2107@columbia.edu>
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Jun 7, 2020
* Add parity test for simple RNN

* Update test_rnn_parity.py

Co-authored-by: William Falcon <waf2107@columbia.edu>
@Borda Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants