Adding invert operator #3065
Adding invert operator #3065
Conversation
There was a problem hiding this comment.
@datumbox thanks for the PR !
I left few comments and I have another remark for autoaugment stuff:
in fixmatch they added a parametrization of invert op with blending: https://github.com/vfdev-5/FixMatch-pytorch/blob/0dc80189be26604734a5c3d563d3be83d4a38d00/ctaugment/ctaugment.py#L98
Should we provide blend as a separate op ? We already do a blending like that for adjust constrast and friends ops...
|
|
||
| _assert_channels(img, [1, 3]) | ||
|
|
||
| max_val = _max_value(img.dtype) |
There was a problem hiding this comment.
If image dtype is float then max_val will be something very large right ? OK, probably, now I understand the questions asked about float dtype and its range as 0-1.
How about adding an option to the method such that user could specify its own max value. By default can be max dtype...
There was a problem hiding this comment.
Yep you are right. I opted for using the same approach as _bend() to determine the bound. I did not optin for passing it as a param to make sure the API is similar as other transformations.
There was a problem hiding this comment.
Hum, this sounds fishy to me for floating point types. We should probably not be doing 1.79769e+308 - value for doubles, nor 3.40282e+38 - value for floats.
Is there any use-case for applying color transforms to native float images (like satellite or medical data)? If not, we should probably assume that float images are within 0-1 range
There was a problem hiding this comment.
For medical imagery I'd use MONAI as reference and it seems like it is possible: https://docs.monai.io/en/latest/transforms.html#intensity . They do not have any assumptions on the range of input data (uint8 or float).
As for satellite imagery, some intensity manipulations in visible spectrum can be also possible.
I think there are 2 ways possible for this op:
- mention in the docs that this op is working on uint8 only and raise exception for float data in the code
- add min/max data range as input args and handle any type.
There was a problem hiding this comment.
That's a great reference, thanks @vfdev-5 !
I think that exposing a min_value / max_value arguments (which defaults to None and has sensible defaults for different data types) is a good trade-off to consider.
We can go in two steps though if we want to be extra cautious, and first raise an exception and then in the future add those extra arguments.
Thoughts?
| Returns: | ||
| PIL Image or Tensor: Randomly color inverted image. | ||
| """ | ||
| if self.get_params() < self.p: |
There was a problem hiding this comment.
We have decided on getting random apply param with static get_params() ?
There was a problem hiding this comment.
I've asked @fmassa and he recommended adding it in the get_params().
There was a problem hiding this comment.
The output of get_params should in principle be the parameters we will be using to apply a transformation.
In this case, it would be True / False for applying the transform or not.
There was a problem hiding this comment.
@fmassa OK and what to do if there are both to sample: transform random params and random apply one ?
There was a problem hiding this comment.
I don't have a good solution for this one yet. It's a similar story as ColorAdjust, and the solutions are not great.
We might need to revisit our transforms story, maybe breaking it down into two types of transforms.
I don't currently have a use-case that requires exposing |
|
I'm closing this PR and I'm going to move the development to the The reason is, there will be 1 PR for each transform and without the ability to stack them I'll either have to wait for each individual PR to be approved and merged before starting the other one or have to deal with a huge number of conflicts as I'm touching the same files. Please do provide your feedback here though as this transform will serve as the prototype for the rest. Thanks! |
Part of #3050