-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
PR: Add RGB_Colorspace
comparisons
#1274
base: develop
Are you sure you want to change the base?
Conversation
@KelSolaar the one failing test looks like a CI issue, can you kick it off again? |
f8c9032
to
e72f266
Compare
I found that I could accomplish my goals by using |
Hello, I was wondering why not using a lambda or partial? Seems like a lot of code to replace one or two lines no? Especially when seeing the usage: |
RGB_Colorspace
comparisons
Yes... This example in the tests is very ugly but it was more useful elsewhere where I am trying to match and compare different RGB_Colorspace objects. I realized that instead I could write a comparison in the RGB Colorspace object. However using partials still gives some issues. For example I'm not really sure what the best route to go is... but I'm leaning towards removeing GammaFunction now and relying on comparing the For reference: this is for some new features I'm adding to https://github.com/OpenLEDEval/OLE-Toolset for later Entertainment Technology Center testing this year. |
Ah, it is really hard to compare objects like that, because they can use different sub-objects but still be equivalent in term of processing. I guess it would depend what type of comparison you are trying to do? |
In the case of something relatively complex like RGBColourSpace I'm thinking that "equivelence" means that the result of XYZ_to_RGB is guaranteed to be the same with the two different inputs, or at least as certain as anything can be in Python. It's definitely a pretty complicated problem I'm trying to handle actually.... but this comparison is very helpful. I could write something else in my tools... but I think this belongs in base colour. |
Current CI issues asside* I will fix those pending further discussion |
Agreed, the issue I see is that there are so many ways to build a callable, e.g., class, lambda, partial, and to wrap them that I don't see a practical way to do that without actually running the functions on some values and check that they are identical.
|
I don't see value checking as a viable option... not all possibilities will be caught... is lambda backed by a type that I could detect similar to partial? There's also this type hint that I'm using somewhere else... I could go through and make some changes to all the tf files we have class CCTF(Protocol):
def __call__(self, vals: ArrayLike, /) -> NDArrayFloat: ... |
If the behavior is a weak false, meaning that spaces might have the same behavior but might not with a strong true that seems useful |
An alternative is to only have def and partials allowed so that it is simpler. |
You mean don't allow set with a lambda? |
Yes, no lambda, no classes, just pure functions or partials. |
I defined all the used gamma functions as module objects, so it should be able to do direct comparisons now. |
For other readers... some discussion accidentally happened on #1275 @KelSolaar Something like this... from functools import partial
from inspect import isfunction
from colour.models.rgb.transfer_functions import exponent, gamma_function
from pytest import raises
class GammaFunc_2_4:
def __call__(self, values):
gamma_function(values, 2.4)
class FakeRGB:
def __init__(self, cctf):
self._cctf = None
self.cctf = cctf
@property
def cctf(self):
return self._cctf
@cctf.setter
def cctf(self, new_value):
if isinstance(new_value, partial):
if not isfunction(new_value.func):
raise ValueError("cctf set with `partial` can only wrap a `def` defined function")
if new_value.args != ():
raise ValueError("cctf set with `partial` can only use keyword arguments")
self._cctf = new_value
return
if isfunction(new_value):
self._cctf = new_value
return
raise TypeError("cctf may only be set with keyword only partials, or a function")
def __eq__(self, other: object) -> bool:
if not isinstance(other, FakeRGB):
return False
if isfunction(self.cctf) and isfunction(other.cctf):
return self.cctf is other.cctf
if isinstance(self.cctf, partial) and isinstance(other.cctf, partial):
return (
self.cctf.func is other.cctf.func and
self.cctf.args == other.cctf.args and
self.cctf.keywords == other.cctf.keywords
)
return False
assert FakeRGB(gamma_function) == FakeRGB(gamma_function)
assert FakeRGB(partial(gamma_function, exponent=2.4)) == FakeRGB(partial(gamma_function, exponent=2.4))
with raises(ValueError):
FakeRGB(partial(GammaFunc_2_4))
with raises(ValueError):
FakeRGB(partial(gamma_function, 2.4))
with raises(TypeError):
FakeRGB(GammaFunc_2_4) This still has some issues where a partial with no args and no kwargs with the same underlying function should be allowede to compare true. But this case can be added... Just want to check in on going this direction with things. |
Still one more issue with It allows for lambda... but we could check Background: |
I think that we have reject everything but >>> pretending_to_be_a_gamma_2_2_lambda = lambda x: gamma(x, 1)
>>> class PretendingToBeAGamma22Class:
... def __new__(cls, a):
... return gamma(a, 1) |
f909476
to
f7204c7
Compare
Summary
Adds a class to encapsulate initialized functionality of gamma_function. Useful for passing around gamma functions in a dynamic way.
Ahead of #1268, needs to be merged first
Preflight
Code Style and Quality
New transformations have been added to the Automatic Colour Conversion Graph.New transformations have been exported to the relevant namespaces, e.g.colour
,colour.models
.Documentation