-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Mux up callback #1241
Mux up callback #1241
Conversation
Hello @Dokholyan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-07-12 15:36:51 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
catalyst/utils/mixup.py
Outdated
augmented batch | ||
|
||
""" | ||
assert isinstance(keys, list), f"keys must be list[str], get: {type(keys)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate it if we could check not for list
s only, but for other iterable
s too, for example, tuple
s
catalyst/utils/mixup.py
Outdated
""" | ||
assert isinstance(keys, list), f"keys must be list[str], get: {type(keys)}" | ||
assert alpha >= 0, "alpha must be>=0" | ||
assert mode in ["add", "replace"], f"mode must be in 'add', 'replace', get: {mode}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate it if we could use tuple
or set
here instead of a list
) | ||
|
||
.. note:: | ||
With running this callback, many metrics (for example, accuracy) become undefined, so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"By running ..." ?
catalyst/callbacks/mixup.py
Outdated
""" | ||
assert isinstance(keys, (str, list)), f"keys must be str of list[str], get: {type(keys)}" | ||
assert alpha >= 0, "alpha must be>=0" | ||
assert mode in ["add", "replace"], f"mode must be in 'add', 'replace', get: {mode}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate it if we could use tuple or set here instead of a list
btw, is it possible to merge this callback with https://github.com/catalyst-team/catalyst/blob/master/catalyst/callbacks/batch_transform.py#L17 ? |
Co-authored-by: Yauheni Kachan <[email protected]>
Co-authored-by: Yauheni Kachan <[email protected]>
Co-authored-by: Yauheni Kachan <[email protected]>
Co-authored-by: Yauheni Kachan <[email protected]>
…into MuxUPCallback
@Scitator about BatchTransformCallback, I thought about it, but I don't know how to introduce а on_train_only mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, @Dokholyan could you please add your update to the Changelog.md?
Before submitting (checklist)
catalyst-make-codestyle && catalyst-check-codestyle
(pip install -U catalyst-codestyle
).make check-docs
?pytest .
?latest
andminimal
requirements?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.
FAQ
Please review the FAQ before submitting an issue: