Feature/periodical validation callback #818
Conversation
|
@Scitator what do you think about adding some message (or warning?) about missing loaders (specified in arguments but not presented in loaders dictionary) ? Like: |
|
@ditwoo I am not sure, if we really need them. I think, the typical usage would be to periodically iterate over valid-like datasets only. That means, that typical usage would consist of only a subset of loader and I don't think we need a warning every epoch/stage during training. |
|
@ditwoo could you please fix docs codestyle? |
| .. note:: | ||
| ``'train'`` is a required loader and will | ||
| be ignored from passed loaders. |
There was a problem hiding this comment.
Hmm, I thought that train is the main training loader, something like valid loader used for collecting validation metrics.
But, seems like it is not required.
There was a problem hiding this comment.
by catalyst design you can pass loaders with any names you want
we have quite simple logic for loaders naming :)
https://github.com/catalyst-team/catalyst/blob/master/catalyst/core/runner.py#L440#L448
| if not isinstance(loader, str) or not isinstance( | ||
| period, (int, float) | ||
| ): |
There was a problem hiding this comment.
what do you think about assert here, to make sure, that all passed params are correct?
I think,it's better to raise an error, rather then silently "forget" about some loader :)
There was a problem hiding this comment.
Yep, seems correct.
What do you think about raising TypeError with some message instead of assert statement?
| state.valid_loader = ( | ||
| self.valid_loader | ||
| if self.valid_loader in epoch_loaders | ||
| else "train" | ||
| ) |
There was a problem hiding this comment.
sorry, but we could not use train loader as valid one;
I suggest to remember valid_metrics from state and pass them for the epochs without valid_loader
There was a problem hiding this comment.
oh, looks like it was required only as a trick around our requirement, that valid_loader in loaders....
in such case I suggest to use just the first key from loaders, rather than fix some name, like train
There was a problem hiding this comment.
Okay, I'll update that and will add information about this behaviour to docs.
docs/api/contrib.rst
Outdated
| PeriodicLoaderRunnerCallback | ||
| """""""""""""""""""""" |
There was a problem hiding this comment.
could you fix docs codestyle?
PeriodicLoaderRunnerCallback
""""""""""""""""""""""""""""""""""
| from catalyst.core import Callback, CallbackOrder, State | ||
|
|
||
|
|
||
| class PeriodicLoaderRunnerCallback(Callback): |
There was a problem hiding this comment.
is it possible to rename it to something without Runner (it could be misleading for users)
PeriodicLoaderRunCallback ? or just PeriodicLoaderCallback? what do you think about it?
There was a problem hiding this comment.
Sure, I'll use PeriodicLoaderCallback
Before submitting
catalyst-make-codestyle && catalyst-check-codestyle(pip install -U catalyst-codestyle).make check-docs?Description
Related Issue
Type of Change
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.
You can use 'Login as guest' to see Teamcity build logs.