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

Change validate_every_epoch (bool) -> validate_every_epoch (int) #27

Closed
wants to merge 2 commits into from

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Dec 2, 2017

New feature as discussed in #16

@alykhantejani
Copy link
Contributor

Thanks, let's also get @fmassa's opinion on this before merging

@fmassa
Copy link
Member

fmassa commented Dec 11, 2017

While I'm ok with this change, I'm thinking about one of the sentences in The Zen of Python, which reads

There should be one-- and preferably only one --obvious way to do it.

In this spirit, we would even remove the validate_every_epoch argument, and recommend using the VALIDATION_STARTING handler to perform the such checks based on epoch number (which would allow the user to perform arbitrarily complex checks given the epoch number).

What do you think?

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Dec 12, 2017

@fmassa I agree to remove validate_every_epoch. However VALIDATION_STARTING handler is less obvious way to do this.

@alykhantejani
Copy link
Contributor

@vfdev-5 @fmassa I don't think VALIDATION_STARTING is the right handler to use if we remove validate_every_epoch. Instead you would want to use TRAINING_ITERATION_COMPLETED or TRAINING_EPOCH_COMPLETED as the VALIDATION_STARTING event is only triggered when trainer.validate() is called

@alykhantejani
Copy link
Contributor

After discussions in #35 and #27 I think there is some consensus on removing this parameter altogether and providing a convenient handler that calls validate and wraps the logic for doing this every x epochs/iterations (see #42).

Closing this issue for now, let's redirect the conversation to #42

@vfdev-5 vfdev-5 deleted the validate_every_epoch branch June 2, 2018 16:57
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