colour-science / colour

Colour Science for Python
https://www.colour-science.org
BSD 3-Clause "New" or "Revised" License
2.14k stars 263 forks source link

PR: Add `RGB_Colorspace` comparisons #1274

Open tjdcs opened 5 months ago

tjdcs commented 5 months ago

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

Documentation

coveralls commented 5 months ago

Coverage Status

coverage: 99.978%. remained the same when pulling f8c9032d83d49c7faa8ab8980be7e213969c180f on feature/gamma_class into 9fe1fdf2bc54935559af8094eec3b012fd001065 on develop.

tjdcs commented 5 months ago

@KelSolaar the one failing test looks like a CI issue, can you kick it off again?

coveralls commented 5 months ago

Coverage Status

coverage: 99.978%. remained the same when pulling e72f266744c79aa39af1362dc10e7befed32485e on feature/gamma_class into b2da242deef631f01c2d1b38f22fa97616965e9f on develop.

tjdcs commented 5 months ago

I found that I could accomplish my goals by using partial but I still prefer this because it is a little but more clear than if isinstance(f,partial) and f.func is gamma_function

KelSolaar commented 5 months ago

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: GammaFunction(2.2, "Preserve")(0.0), this is not super pretty :(

tjdcs commented 5 months ago

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: GammaFunction(2.2, "Preserve")(0.0), this is not super pretty :(

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 partial(gamma_function,2.2) and partial(gamma_function, exponent=2.2 should match, but don't without further metaanalysis of the underlying function. Encapsulating these parameters in a class makes for easier comparisons.

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 partial objects even though they might be a little bit un-intuitive in cases where users mix args and kwargs.

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.

KelSolaar commented 5 months ago

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?

tjdcs commented 5 months ago

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.

tjdcs commented 5 months ago

Current CI issues asside* I will fix those pending further discussion

KelSolaar commented 5 months ago

but I think this belongs in base colour.

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.

>>> from functools import partial
>>> def gamma(a, b):
...     return a ** b
...
>>> def g(a):
...     return gamma(a, 2)
...
>>> p = partial(gamma, 2)
>>> l = lambda x: gamma(x, 2)
>>> class C:
...     def __new__(cls, a):
...         return gamma(a, 2)
...
>>> p(2)
4
>>> l(2)
4
>>> g(2)
4
>>> C(2)
4
tjdcs commented 5 months ago

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: ...
tjdcs commented 5 months ago

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

KelSolaar commented 5 months ago

An alternative is to only have def and partials allowed so that it is simpler.

tjdcs commented 5 months ago

You mean don't allow set with a lambda?

KelSolaar commented 5 months ago

Yes, no lambda, no classes, just pure functions or partials.

KelSolaar commented 5 months ago

I defined all the used gamma functions as module objects, so it should be able to do direct comparisons now.

tjdcs commented 5 months ago

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.

tjdcs commented 5 months ago

Still one more issue with isfunction (at least)

It allows for lambda... but we could check new_value.__name__ and reject '<lambda>'

Background: https://stackoverflow.com/a/20059029/2700516 https://stackoverflow.com/a/24578359/2700516

KelSolaar commented 5 months ago

I think that we have reject everything but def and partial: We cannot really cheat with a partial but can easily with a lambda or a class:

>>> pretending_to_be_a_gamma_2_2_lambda = lambda x: gamma(x, 1)
>>> class PretendingToBeAGamma22Class:
...     def __new__(cls, a):
...         return gamma(a, 1)