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

rename Model steps #1051

Merged
merged 37 commits into from
Mar 5, 2020
Merged

rename Model steps #1051

merged 37 commits into from
Mar 5, 2020

Conversation

williamFalcon
Copy link
Contributor

@williamFalcon williamFalcon commented Mar 5, 2020

What does this PR do?

Fixes # (issue).

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 Mar 5, 2020

Hello @williamFalcon! Thanks for updating this PR.

Line 453:101: E501 line too long (113 > 100 characters)
Line 465:101: E501 line too long (109 > 100 characters)
Line 517:101: E501 line too long (105 > 100 characters)
Line 530:101: E501 line too long (105 > 100 characters)
Line 533:101: E501 line too long (104 > 100 characters)
Line 580:101: E501 line too long (102 > 100 characters)
Line 590:101: E501 line too long (111 > 100 characters)
Line 591:101: E501 line too long (117 > 100 characters)
Line 677:101: E501 line too long (107 > 100 characters)
Line 689:101: E501 line too long (109 > 100 characters)

Comment last updated at 2020-03-05 17:32:23 UTC

@Borda Borda added the feature Is an improvement or enhancement label Mar 5, 2020
@Borda Borda added this to the 0.7.0 milestone Mar 5, 2020
Copy link
Member

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

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

Minor comment in the docs, everything else looks great, and should be backwards compatible :)

@@ -34,11 +34,11 @@ Log metrics

To plot metrics into whatever logger you passed in (tensorboard, comet, neptune, etc...)

1. Training_end, validation_end, test_end will all log anything in the "log" key of the return dict.
1. training_step_end, validation_end, test_end will all log anything in the "log" key of the return dict.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps these should say validation_epoch_end or validation_step_end?

@awaelchli
Copy link
Contributor

Will validation_end be renamed to validation_epoch_end? Validation can be done multiple times per epoch by setting the flag in the trainer, so renaming that would be confusing.

@williamFalcon
Copy link
Contributor Author

williamFalcon commented Mar 5, 2020

Will validation_end be renamed to validation_epoch_end? Validation can be done multiple times per epoch by setting the flag in the trainer, so renaming that would be confusing.

yes, but validation_epoch_end is called at the end of a validation epoch NOT training epoch.
But I could see how it might be confusing. Is it better to explain in docs?

the general new pattern is:

___step
___step_end
___epoch_end

__step_end is only necessary if you use dp and need full batch loss or something (ie: negative sampling)

@williamFalcon williamFalcon merged commit 29faea1 into master Mar 5, 2020
@Borda Borda deleted the steps branch March 5, 2020 19:19
@Borda Borda changed the title Steps rename Model steps Mar 8, 2020
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* training_end renamed to training_step_end

* training_end renamed to training_step_end

* training_end renamed to training_step_end

* training_end renamed to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* fix lost model reference

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end

* training_end to training_step_end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants