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: Uniquely define common gamma functions using partial. #1275

Merged
merged 1 commit into from
Jun 22, 2024

Conversation

KelSolaar
Copy link
Member

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

  • Unit tests have been implemented and passed.
  • Pyright static checking has been run and passed.
  • Pre-commit hooks have been run and passed.
  • [N/A] New transformations have been added to the Automatic Colour Conversion Graph.
  • [N/A] 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.

@KelSolaar KelSolaar added this to the v0.4.5 milestone Jun 22, 2024
@coveralls
Copy link

Coverage Status

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

@KelSolaar KelSolaar merged commit d889366 into develop Jun 22, 2024
26 checks passed
@KelSolaar KelSolaar deleted the feature/gamma_partials branch June 22, 2024 23:42
@tjdcs
Copy link
Contributor

tjdcs commented Jun 23, 2024

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

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

For example:

>>> colour.models.RGB_COLOURSPACE_DON_RGB_4.cctf_encoding is colour.models.RGB_COLOURSPACE_RUSSELL_RGB.cctf_encoding
True

@tjdcs
Copy link
Contributor

tjdcs commented Jun 23, 2024

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

tjdcs commented Jun 23, 2024

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

@tjdcs
Copy link
Contributor

tjdcs commented Jun 23, 2024

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

KelSolaar commented Jun 23, 2024

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

tjdcs commented Jun 23, 2024

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

tjdcs commented Jun 23, 2024

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

tjdcs commented Jun 23, 2024

Sorry I meant to have this discussion on #1274

@colour-science colour-science locked and limited conversation to collaborators Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants