-
Notifications
You must be signed in to change notification settings - Fork 624
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
Automatic augmentations gallery #4648
Conversation
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 would add a jupyter notebook to show how to use it.
f3fa6f0
to
fc904f4
Compare
!build |
CI MESSAGE: [7295532]: BUILD STARTED |
CI MESSAGE: [7295532]: BUILD FAILED |
mag_range : (int, int) | ||
Specifies the range of applicable magnitudes for the operation. | ||
randomly_negate: bool | ||
If true, the magnitude from the mag_range will be randomly negated for every sample. |
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.
Just a note, but this may be an interesting distinction between the magnitude being negated or the parameter being negated, as it might not hold that as_param(-magnitude) = -as_param(magnitude)
. And this may be a source of potential errors.
|
||
def __init__(self, op, mag_range=None, randomly_negate=None, as_param=None, param_device=None): | ||
self._op = op | ||
self._mag_range = mag_range |
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.
Just a note, but maybe it is good idea to include some strict checks of what is in the mag_range, so we end up with less cryptic error messages?
config = {name: value for name, value in self._get_config()} | ||
for name, value in ( | ||
('mag_range', mag_range), | ||
('as_param', as_param), | ||
('randomly_negate', randomly_negate), | ||
('param_device', param_device), | ||
): | ||
if value is not _DummyParam: | ||
config[name] = value | ||
return cls(self._op, **config) | ||
|
||
def _get_config(self): | ||
return [(name, value) for name, value in ( | ||
('mag_range', self._mag_range), | ||
('as_param', self._as_param), | ||
('randomly_negate', self._randomly_negate), | ||
('param_device', self._param_device), | ||
)] |
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.
Just a nitpick, this looks a bit repetititive.
def __repr__(self): | ||
aug_params_repr = [repr(self._op)] | ||
config = [(name, val) for name, val in self._get_config() if val is not None] | ||
config_reprs = [f"{name}={repr(param)}" for name, param in config] | ||
aug_params_repr.extend(config_reprs) | ||
return f"Augmentation({', '.join(aug_params_repr)})" |
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 wanted to suggest that we should have a good repr that shows the argument <-> magnitude mappings, if we add the symmetry via negation + possibly the name of the operation somehow. This is a good start.
def param_device(self): | ||
return self._param_device or "cpu" | ||
|
||
def augmentation(self, mag_range=_DummyParam, randomly_negate=_DummyParam, as_param=_DummyParam, |
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.
Hmm, neat. If we intend to use this as a function, maybe we should give it a bit different name though? Just a though.
Another thought I had was to just allow @augmentation
decorator stacking, this is possibly a bit more tricky for us, but maybe it would be more consistent for the user?
Basically:
@augmentation(mag_range=(0, 100)
@augmentation(mag_range=(0, 10)
def foo(....)
pass
should work, but the usage would be to decorate already existing one?
OTOH, it's less convenient as you don't get the result as a return value and are better off just calling the decorator directly to capture the new callable.
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.
It does not hurt to allow nesting, I can adjust that. As for other names - I don't have a good candidate. And the augmentation name has the benefit of being consitent with the decorator name.
The `@augmentation` lets you to specify what should the range of magnitudes and if the magnitude | ||
should be randomly negated. Additionally, the `as_param` helps to separate the computation of |
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.
And I think our wording should be a bit different. (this is a bit nitpicking, and doesn't take away from the implementation).
We have magnitude space: [0, 1, ..., n], mapped into parameter range [0.0, p). The range of magnitudes would be selected by the number of bins, the range of the applied parameter is customizable.
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.
The AA paper states
We discretize the range of magnitudes into 10 values (uniform spacing) so that we can use
a discrete search algorithm to find them.
so there, the [0.0, p] is the magnitude range. Overall it is a bit ambigious, but instead of renaming the magnitudes into parameters I'd rather put more emphasis on magnitude and magnitude bins. How about such distinction?
|
||
The augmentations in this module are defined with some default ranges passed to `@augmentation`. | ||
The parameters can be easily adjusted. For example, to increase the magnitudes range | ||
of `shear_x`, you can create `my_shear_x = shear_x.augmentation(mag_range=(0, 0.5))`. |
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.
If there is randomly_negate=True, should we check if the user passed a range that makes sense - that is (0, something)?
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 don't think so. ((-10, 10), False) and ((-10, 10), True) for AA or RA result in different runtime magnitudes. One or another coud make sense. Or (-5, 15) etc.
[('shear_x', 0.0, 0), ('solarize', 0.8, 4)], | ||
[('shear_y', 0.8, 0), ('color', 0.6, 4)], | ||
[('color', 1.0, 0), ('rotate', 0.6, 2)], | ||
[('equalize', 0.8, 4)], |
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.
What happened here?
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.
Originally it is equalize with 0 probability of being run followed by equalize with 0.8 probability.
|
||
auto_augment_image_net_policy = Policy( | ||
"ImageNet", 11, { | ||
"shear_x": a.shear_x.augmentation((0, 0.3), True), |
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.
Should the @augmentation
get optional name argument otherwise infer it from the function name, so we can get neat __repr__
for all that stuff?
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.
The name idea is nice, we could accept here just a list and make a dict of it.
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.
On the other hand, it kind of compilcates things I guess. I just take the name (__name__
) of the decorated function.
"invert": a.invert, | ||
"equalize": a.equalize, | ||
"auto_contrast": a.auto_contrast, | ||
}, [ |
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 guess there is no escape from the dict that has all the usable operations grouped up.
It's both nice for us (we can for example print them), and a bit of a shame (it was a bit shorter and more direct).
93154ab
to
95af95f
Compare
!build |
CI MESSAGE: [7309341]: BUILD STARTED |
CI MESSAGE: [7310181]: BUILD STARTED |
CI MESSAGE: [7309341]: BUILD FAILED |
CI MESSAGE: [7310181]: BUILD PASSED |
!build |
CI MESSAGE: [7377149]: BUILD STARTED |
CI MESSAGE: [7377149]: BUILD FAILED |
!build |
CI MESSAGE: [7389682]: BUILD STARTED |
CI MESSAGE: [7389682]: BUILD PASSED |
!build |
CI MESSAGE: [7391042]: BUILD STARTED |
CI MESSAGE: [7391042]: BUILD PASSED |
382a760
to
5960f74
Compare
b466118
to
24b0ac6
Compare
Signed-off-by: Kamil Tokarski <[email protected]> Rename the selection operator Signed-off-by: Kamil Tokarski <[email protected]> Use random.uniform discrete mode Signed-off-by: Kamil Tokarski <[email protected]> Pass kwargs directly Signed-off-by: Kamil Tokarski <[email protected]> Assure uniform choice of manitudes in TA Signed-off-by: Kamil Tokarski <[email protected]> AutoAugment: coallsce pollicies Signed-off-by: Kamil Tokarski <[email protected]> Avoid unnecessary casts in color operations expressed in terms of arithm ops Signed-off-by: Kamil Tokarski <[email protected]> AutoContrast, one less float32 intermediate result Signed-off-by: Kamil Tokarski <[email protected]> Expose select operator Signed-off-by: Kamil Tokarski <[email protected]> Review rework, usage lessons learnt from the expiriments Signed-off-by: Kamil Tokarski <[email protected]> Adjust the docs Signed-off-by: Kamil Tokarski <[email protected]> Better handling of non-unique names, better custom kwargs handling, improved error messaging Signed-off-by: Kamil Tokarski <[email protected]> Rework pretty_select and utils Signed-off-by: Kamil Tokarski <[email protected]> Linter complaints Signed-off-by: Kamil Tokarski <[email protected]> Private modules Signed-off-by: Kamil Tokarski <[email protected]> Simplify docs, simplify code Signed-off-by: Kamil Tokarski <[email protected]> Move policy to separate module Signed-off-by: Kamil Tokarski <[email protected]> More (and more intelegible) docs Signed-off-by: Kamil Tokarski <[email protected]> Type annotations Signed-off-by: Kamil Tokarski <[email protected]> Improved error reporting for as_param Signed-off-by: Kamil Tokarski <[email protected]> More specific callable annotations Signed-off-by: Kamil Tokarski <[email protected]> Polish names Signed-off-by: Kamil Tokarski <[email protected]> Better error messaging, polishing Signed-off-by: Kamil Tokarski <[email protected]> Better error-reporting, warnings, default random_sign Signed-off-by: Kamil Tokarski <[email protected]> Make Aug private Signed-off-by: Kamil Tokarski <[email protected]> Wrapper tests Signed-off-by: Kamil Tokarski <[email protected]> More tests Signed-off-by: Kamil Tokarski <[email protected]> More tests Signed-off-by: Kamil Tokarski <[email protected]> Test select Signed-off-by: Kamil Tokarski <[email protected]> More tests Signed-off-by: Kamil Tokarski <[email protected]> Shape passing fix Signed-off-by: Kamil Tokarski <[email protected]> More tests Signed-off-by: Kamil Tokarski <[email protected]> More tests Signed-off-by: Kamil Tokarski <[email protected]> Even more tests Signed-off-by: Kamil Tokarski <[email protected]> Even even more Signed-off-by: Kamil Tokarski <[email protected]> Fix a comment Signed-off-by: Kamil Tokarski <[email protected]> Fix linting issues Signed-off-by: Kamil Tokarski <[email protected]> Replace contrast with arithm ops Signed-off-by: Kamil Tokarski <[email protected]> Signed magnitude wrapper Signed-off-by: Kamil Tokarski <[email protected]>
Signed-off-by: Kamil Tokarski <[email protected]>
Signed-off-by: Kamil Tokarski <[email protected]>
Signed-off-by: Kamil Tokarski <[email protected]>
da572bc
to
d4f4bb5
Compare
Signed-off-by: Kamil Tokarski <[email protected]>
Signed-off-by: Kamil Tokarski [email protected]
Category:
Description:
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A