Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
BrightnessContrast operator #1188
BrightnessContrast operator #1188
Changes from 41 commits
175b768
67ecae2
ca22d38
0bc93c5
09f84b2
dab3536
b9a6700
e28f50b
fe5d742
93fb518
4735bb1
a1c6120
9b33163
f90d642
026ea07
2f99aeb
3101bc9
b36e873
0237760
615bde0
3aa2201
d95cde5
3103c9e
289ac75
10bbed7
e8ef7cf
cc53f98
3f0f84d
370a83b
5462d4f
4b147b0
2056384
3ed1f6c
122d154
7abd61f
0d8a6f2
1f63950
f07d894
c28402b
f31167b
0a6cc20
43cb965
5e2a9c8
c058a6c
0a66242
90bd1eb
cc4f7d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 personally dislike the use of the constant here. I'd like to see the name of the argument 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.
The string with argument name needs to be used multiple times in implementation of operator. In this case, I want to have it as a constant, no a magic string
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 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.
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.
Changed to
spec::kBrightness
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.
DALI_TYPE_SWITCH_WITH_FP16 ?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says:
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
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's your opinion on calling
kernel::Setup
in Operator::SetupImpl`?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.
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
inSetupImpl
. On the other hand, implementing it this way would require major change with duplicatingTYPE_SWITCH
es, 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.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.
Maybe personal preference:
A little bit of an overkill in my opinion. I'd rather have
vector<float>
all the time and get rid ofargument_t
. It will just have 1 element for CPUBackend and more than that for GPUBackend