Fix percent_checks #649
Fix percent_checks #649
Conversation
| @@ -48,15 +48,30 @@ def init_train_dataloader(self, model): | |||
| if EXIST_ITER_DATASET and isinstance(self.get_train_dataloader().dataset, IterableDataset): | |||
| self.num_training_batches = float('inf') | |||
| else: | |||
| if not 0. <= self.train_percent_check <= 1.: | |||
| raise ValueError(f"train_percent_check must lie in the range [0.0, 1.0], but got " | |||
There was a problem hiding this comment.
pls use ` around functions and variables...
| self.num_training_batches = len(self.get_train_dataloader()) | ||
| self.num_training_batches = int(self.num_training_batches * self.train_percent_check) | ||
| self.num_training_batches = max(1, self.num_training_batches) |
There was a problem hiding this comment.
I would consider adding a check and optionaly raise error if the nb is 0 becase with empty dataloader it may crash later anyway...
There was a problem hiding this comment.
After the #653 fix there should be no problems with empty dataloaders for now. So I decided to remove max(1, ...) for training and test batches.
But I agree that maybe we should better think about the cases when when num_training_batches=0 or len(train_dataloader)=0. It is not very straightforward because the user, for example, can define the model with only test_step and test_end and test_dataloader for testing. In that case an empty train_dataloader is acceptable.
|
|
||
| # determine number of validation batches | ||
| # val datasets could be none, 1 or 2+ | ||
| if self.get_val_dataloaders() is not None: | ||
| if not 0. <= self.val_percent_check <= 1.: |
There was a problem hiding this comment.
as it is relative pattern you may move this check to a private method and call it here instaed
|
|
||
| # determine number of validation batches | ||
| # val datasets could be none, 1 or 2+ | ||
| if self.get_val_dataloaders() is not None: | ||
| if not 0. <= self.val_percent_check <= 1.: |
There was a problem hiding this comment.
as it is a relative pattern you may move this check to a private method and call it here instead so when the text is changed later it will be stil consistent...
| f"val_check_interval ({self.val_check_interval}) must be less than or equal to " | ||
| f"the number of the training batches ({self.num_training_batches}). " | ||
| f"If you want to disable validation set val_percent_check to 0.0 instead.") | ||
| f"`val_check_interval` ({self.val_check_interval}) must be less than or equal " |
There was a problem hiding this comment.
pls pep8 prefers to have whitespace on the beginning of next line instead of the last line ending
There was a problem hiding this comment.
Sorry, I haven't fixed it yet and PR has been already merged. But I got your point. From now on I will not use trailing spaces in multiline strings :)
There was a problem hiding this comment.
well if you feel like nothing to do, you may check all string in the package... ;]
This PR resolves #646 and resolves #631
In short:
val_percent_check=0.num_training_batchesandnum_test_batchesare always not less than 1.val_check_intervalis checked to be in the range [0.0, 1.0] or to be not greater thannum_training_batches.