Skip to content
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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

tjdcs
Copy link
Contributor

@tjdcs tjdcs commented Jun 20, 2024

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

  • Unit tests have been implemented and passed.
  • Pyright static checking has been run and passed.
  • Pre-commit hooks have been run and passed.
  • 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

  • New features are documented along with examples if relevant.
  • The documentation is Sphinx and numpydoc compliant.

@tjdcs tjdcs requested a review from KelSolaar June 20, 2024 02:02
@coveralls
Copy link

Coverage Status

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

@tjdcs
Copy link
Contributor Author

tjdcs commented Jun 20, 2024

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

@coveralls
Copy link

Coverage Status

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

@tjdcs
Copy link
Contributor Author

tjdcs commented Jun 20, 2024

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
Copy link
Member

KelSolaar commented Jun 20, 2024

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 tjdcs changed the title PR: Add encapsulation class for gamma_function PR: Add RGB_Colorspace comparisons Jun 21, 2024
@tjdcs
Copy link
Contributor Author

tjdcs commented Jun 21, 2024

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
Copy link
Member

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
Copy link
Contributor Author

tjdcs commented Jun 21, 2024

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
Copy link
Contributor Author

tjdcs commented Jun 21, 2024

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

@KelSolaar
Copy link
Member

KelSolaar commented Jun 21, 2024

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
Copy link
Contributor Author

tjdcs commented Jun 21, 2024

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
Copy link
Contributor Author

tjdcs commented Jun 21, 2024

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
Copy link
Member

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

@tjdcs
Copy link
Contributor Author

tjdcs commented Jun 21, 2024

You mean don't allow set with a lambda?

@KelSolaar
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants