Rename and fix overfit_pct, val_percent_check and test_percent_check

[williamFalcon]

Fixes #1668
Fixes #1920
Fixes #830

Overfit

• Removes overfit_pct
• Adds overfit_batches which can be a float (%) or int (steps)
• overfit now makes sure train, test and val splits are all the train step
• sets shuffle=False in the train set

limit_xxx_batches

• removes train_percent_check and val_percent_check
• replaces with limit_train_batches and limit_val_batches which can also be int (steps) or percent (float)

docs + tests

• adds tests to verify
• updates docs

@tullie these flag names make sense?

TODO: also replace train_percent_check with limit_train_batches

[pep8speaks]

Hello @williamFalcon! Thanks for updating this PR.

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

Comment last updated at 2020-06-17 11:54:19 UTC

[awaelchli]

@williamFalcon are you trying to fix this: #1920 ?

[williamFalcon]

@awaelchli partly. also enabling this to take a number of batches instead of just a percent.

[awaelchli]

besides the new Trainer flags, it's not easy to see what's the difference to #1920. It seems you have re-implemented it.

[Borda]

@williamFalcon are you trying to fix this: #1920 ?

@awaelchli partly. also enabling this to take a number of batches instead of just a percent.

so shall #1920 be merged first? otherwise, it may raise plenty of conflicts...

[Borda]

let's merge #1920 first 🐰

[mergify]

This pull request is now in conflict... :(

[codecov]

Codecov Report

Merging #2213 into master will decrease coverage by 0%.
The diff coverage is 88%.

@@          Coverage Diff           @@
##           master   #2213   +/-   ##
======================================
- Coverage      88%     88%   -0%     
======================================
  Files          69      69           
  Lines        5269    5289   +20     
======================================
+ Hits         4625    4637   +12     
- Misses        644     652    +8     

[Borda]

btw, this was not reviewed as @awaelchli @Borda asked to rebase it on #1920
btw2, there are many unrelated changes, probably artefacts from merging master

[tullie]

I wonder if overfit_batches can be removed entirely. It's good to strive for an API where there's only one way to do things, and I feel like the problem with these trainer args is that it's not clear what the difference between setting overfit_batches=1 or limit_val_batches=1 and limit_train_val=1 is?

Regardless, this PR is still an improvement to how it was before. Is there any reason you didn't put deprecation warnings in?

[williamFalcon]

This was a two-part PR.
The warnings are here #2220

This was the time to remove overfit but i realized it actually serves a different approach.

overfit_batches will actually use the train set for train, val and test so you can truly overfit. It also removes the shuffle from the training set so you iterate over the same exact stuff.

However, the other 3 flags don't serve that purpose. Those are more for only using some of the data for training, val or test (likely for debugging purposes)

[pwl]

Hi, I'm getting an error

~/.local/lib/python3.7/site-packages/pytorch_lightning/callbacks/progress.py in total_val_batches(self)
     99             is_val_epoch = trainer.current_epoch % trainer.check_val_every_n_epoch == 0
    100             total_val_batches = trainer.num_val_batches if is_val_epoch else 0
--> 101             total_val_batches = sum(total_val_batches)
    102         return total_val_batches
    103 

TypeError: 'int' object is not iterable

from a line added in this PR: 04c794c#diff-c611176c04f224f1ec16a768aec9c527R101

Turns out sum(0) doesn't work in python, so maybe it should be

total_val_batches = sum(trainer.num_val_batches) if is_val_epoch else 0

I'm writing this here instead in a new issue, as this is a very fresh PR, I hope that's ok. Also, I wonder how this got past the tests.

[Borda]

@pwl good catch, mind sending a fix PR?