colour-science / colour

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

PR: Uniquely define common gamma functions using partial. #1275

Closed KelSolaar closed 3 weeks ago

KelSolaar commented 3 weeks ago

Summary

This PR uniquely define our common gamma function using partial.

The intent is to enable direct colour.RGB_Colourspace class instance as implemented in #1274.

Preflight

Code Style and Quality

Documentation

coveralls commented 3 weeks ago

Coverage Status

coverage: 99.978%. remained the same when pulling 23f045c05f0cffa5fc5c2422b5cc3cf9df4e2ac9 on feature/gamma_partials into b2da242deef631f01c2d1b38f22fa97616965e9f on develop.

tjdcs commented 3 weeks ago

While this might be nice for some convenience and clarity in user code, because these are partials they will still trigger the isinstance(obj, partial) path. Partials have public func, args, and keywords that I can compare.

So... if it's helpful to user code, this is a good change but not a necessary one I think. I should be able to do more code soon to clarify.

KelSolaar commented 3 weeks ago

But you should not need to check whether they are partial, you can check whether they are the same object, i.e., func_1 is func_2. I note also that in your PR you do equality comparisons but when checking if objects are the same you should use is and not ==. With the new module attribute based partial, this should be possible now.

KelSolaar commented 3 weeks ago

For example:

>>> colour.models.RGB_COLOURSPACE_DON_RGB_4.cctf_encoding is colour.models.RGB_COLOURSPACE_RUSSELL_RGB.cctf_encoding
True
tjdcs commented 3 weeks ago

Yes but I am not comparing the standard color spaces. I need to compare CCTF functions with "non-standard" properties. Checking partials and their properties is required for my application.

Noted about using is for object comparison... good point.

tjdcs commented 3 weeks ago

Checking the 3 properties of partial objects is not difficult. But can still lead to false negatives.

tjdcs commented 3 weeks ago

If I was comparing the standard objects I could also just use is and not need __eq__ at all... The semantics of equality are not the same as the semantics of being the same object. Objects might have equivalent behavior but not be the same object which is what I need to compare in the case of two RGB_Colorspaces with non-standard primaries and non-standard cctfs.

KelSolaar commented 3 weeks ago

if I was comparing the standard objects I could also just use is and not need eq at all...

For sure but the only safe way to check whether a callable is the same is to use is for that comparison.

We can either check maximally for functionality by crunching numbers which might be impossible or minimally with is.

All those objects are functionally the same but we are saying that our equality check would miss the lambda and the class:

>>> 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)

This does not feel right and is super flaky at best. Rather than rolling this in colour directly, an alternative could be to monkey-patch inside the dependent library application? I would rather have a "clean" comparison in Colour, e.g., numerical values are checked with equality and objects with is, and if another library needs a relaxed method, it can reimplement something more lenient.

tjdcs commented 3 weeks ago

I don’t think false negatives are harmful for comparing such a complicated object. And checking that partials have the same underlying function object and the same arguments or that the cctf is the same function argument seems fine. I can’t think of false positives in this case.

Especially if the cctf setter functions reject anything other than a function or a partial, which I mentioned above.

I would be impossible to check numerically and prevent false positives so that is not an option.

tjdcs commented 3 weeks ago

Actually, if the setter rejects any other values… the only functions that could case false negatives are partials that mix positional vs keyword arguments. Which we could reject in the setter.

Let me come back in a few days with a different proposal.

tjdcs commented 3 weeks ago

Sorry I meant to have this discussion on #1274