-
Notifications
You must be signed in to change notification settings - Fork 679
change invert() and add invert_in_place() #2438
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
base: main
Are you sure you want to change the base?
Conversation
|
This is a breaking change, so at a minimum it would have to wait for our next major release. However, I'm also not sure whether this change is worthwhile. Switching to the I'd probably hold off on the |
|
I changed the design for no breaking change Still I think that not having the same API for method is not great. Maybe deprecated |
|
I also think that a breaking change is a good option with the version bumped Note great for current users of |
|
I believe before adding any new processing methods iterators at the very least need to get sorted out. |
|
We've been intermittently discussing a possible overhaul of the What is this PR trying to achieve? My interpretation is that it is about adding consistency to the colorops module. Currently, the But stepping back, is having two versions of every method really the approach we'd pick starting from scratch? We'd probably just have the in-place versions, right? Which we could do in the next major release by dropping all the |
|
In the meantime, we do have some evidence that in-place transformations (or rather in-allocation) are generally desirable: #2650. I think more people stumbled upon it but simply did not ask and accepted the allocation. That said there is a bit of design space on how to achieve them since it's correctly observed that we do need two separate APIs for aliasing reasons when we take If we spin this thread of thought further, we could possibly have So:
Something as simple as |
This PR copy the design of
flip_horizontal()forinvert()Invert is the only inplace function
With this PR there are now
invert()andinvert_in_place()