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
feat(IBA): IBA::scale() #4541
feat(IBA): IBA::scale() #4541
Changes from 1 commit
e496ab1
39957b6
a07bddf
a106ab8
9c4ec64
b7eef84
bb7824f
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.
This is a behaviour changes for mul(). Maybe that's fine, but we should certainly document it, and maybe think hard about if we want it.
Are we prepared to say that any time somebody multiplies an n-channel image by a 1-channel image, they mean to scale all channels by that one value? Is there ever a case where people would not want that? (I honestly don't know.)
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 have a strong opinion on that one. Happy to omit that bit and just go with the new algo.
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 does look weird when the alpha is scaled, as you mentioned, but on the other hand this behaviour is consistent with mul() used with a single scalar.
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 think that's fine.
I wonder... if maybe we should get in the habit of adding a KWArgs parameter to all new IBA functions (and, slowly, adding them to exiting ones when the occasion pops up that we have to modify them for other reasons), even if we don't have any keyword args to respond to at this moment, as a placeholder for future expansion without needing to alter the ABIs or add more functions. In this case, that would make it more palatable to pick a reasonable default behavior (just scale all channels) for now and then in the future, we can very easily accommodate an option that would cause the alpha to not be scaled.
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.
We could add a KWArgs parameter to scale(), ignored for now; and another one to mult(), with an option to override the existing behaviour when the second (or any?) argument is single channel. I'm happy to make this change and update the docs if you think this is worth doing now.
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.
Let's not add it to mul (which would either break compatibility, or cause yet another set of mul versions to be added) until we have a desired use.
But let's add it to scale() now -- it costs nothing and has no compatibility issues for a "new" IBA function, and future-proofs the API.
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've added a KWArgs parameter to scale(), and removed the changes from mul() for now.