BrightnessContrast operator #1188
Conversation
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
|
!build |
|
CI MESSAGE: [903753]: BUILD STARTED |
|
CI MESSAGE: [903753]: BUILD FAILED |
|
!build |
|
CI MESSAGE: [903760]: BUILD STARTED |
|
CI MESSAGE: [903760]: BUILD FAILED |
| const auto &input = ws.template Input<Backend>(0); | ||
| auto &output = ws.template Output<Backend>(0); | ||
|
|
||
| TYPE_SWITCH(input.type().id(), type2id, InputType, (uint8_t, int16_t, int32_t, float), ( |
There was a problem hiding this comment.
DALI_TYPE_SWITCH_WITH_FP16 ?
There was a problem hiding this comment.
Regarding types, that are supported here, I followed requirements, so I'd like to manually specify types, that are supported
There was a problem hiding this comment.
It says:
Output types: [u8, i8, u16, i16, u32, i32, f32]
Input types: [u8, i8, u16, i16, u32, i32, f32]
you are not covering all the cases.
In my opinion, there is no reason for not using the DALI_TYPE_SWITCH_WITH_FP16 here and cover all available types
|
!build |
|
CI MESSAGE: [905683]: BUILD STARTED |
|
CI MESSAGE: [905683]: BUILD PASSED |
| Additionally, this operator can change the type of data.)code") | ||
| .NumInput(1) | ||
| .NumOutput(1) | ||
| .AddOptionalArg(detail::kBrightness, |
There was a problem hiding this comment.
I was thinking about that problem. I'd rather make it belong to Operator class than to detail namespace. I would at least change that.
Still it's not aligned with the rest of the code base.
| auto tvout = view<OutputType, 3>(output); | ||
| kernel_manager_.Initialize<BrightnessContrastKernel>(); | ||
| kernels::KernelContext ctx; | ||
| kernel_manager_.Setup<BrightnessContrastKernel>(ws.data_idx(), |
There was a problem hiding this comment.
What's your opinion on calling kernel::Setup in Operator::SetupImpl`?
There was a problem hiding this comment.
Generally speaking, I'd say that it should be this way. However, here we have a special case. Meaning, it's not necessary to call Setup in SetupImpl. On the other hand, implementing it this way would require major change with duplicating TYPE_SWITCHes, which obfuscates the code. While it would be possible to do this implementation without such duplication, the abstraction layer for this is not ready yet and will require some time to evolve. Keeping all this in mind, plus taking into account, that this PR is already quite a time here (I already rebased it twice), I'd like to merge it now and, when it proves to be necessary, add the abstraction layer later.
|
|
||
| private: | ||
| USE_OPERATOR_MEMBERS(); | ||
| detail::argument_t<Backend> brightness_, contrast_; |
There was a problem hiding this comment.
Maybe personal preference:
A little bit of an overkill in my opinion. I'd rather have vector<float> all the time and get rid of argument_t. It will just have 1 element for CPUBackend and more than that for GPUBackend
Co-Authored-By: Joaquin Anton <j.anton.guirao@gmail.com>
|
!build |
|
CI MESSAGE: [916886]: BUILD STARTED |
|
CI MESSAGE: [916886]: BUILD FAILED |
|
CI MESSAGE: [916886]: BUILD PASSED |
* BrightnessContrast operator Signed-off-by: Michał Szołucha <mszolucha@nvidia.com> Co-Authored-By: Joaquin Anton <j.anton.guirao@gmail.com> Signed-off-by: Jianjun Liu <00liujj@163.com>
Why we need this PR?
Pick one
What happened in this PR?
Added operator plus few utilities
JIRA TASK: [DALI-821]