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: Implement support for *HyCH* large color difference #1307

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

mesvam
Copy link
Contributor

@mesvam mesvam commented Oct 30, 2024

Summary

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.

@mesvam
Copy link
Contributor Author

mesvam commented Oct 30, 2024

I think I'm going to need a bit of help with HyCH since it's a bit more complicated than HyAB, and I don't work in this field so I'm not confident in my knowledge.

So first, I'm not sure what's the best way to implement this. If I'm interpreting the equation in the paper correctly, I think HyCH would duplicate the majority of code in delta_E_CIE2000. Should both delta_E_CIE2000 and delta_E_HyCH both call a helper function then to avoid duplication?

Also, Equation 6 for CD4 is notated using L*C*H* in the paper, but I'm seeing L'C'H' used in other sources and also in this package. I'm assuming when the paper uses ΔC*, they mean ΔC' because of the k_C * S_C denominator, but I'm not confident in this. And also since the paper doesn't provide any examples, I can't find any sample data to compare results to verify my implementation.

@KelSolaar KelSolaar deleted the branch colour-science:hych_staging November 1, 2024 19:58
@KelSolaar KelSolaar closed this Nov 1, 2024
@KelSolaar KelSolaar reopened this Nov 1, 2024
@KelSolaar KelSolaar changed the base branch from feature/hyab_staging to develop November 1, 2024 20:02
@KelSolaar
Copy link
Member

Hey @mesvam,

I squashed your commits from the previous branch so you would need to create a new branch from develop, cherry-pick 21823ba, and force-push that branch back to mesvam:feature/HyCH.

I will take a look at the paper but reading what you wrote LCH should really just be Lab converted to LCHab. We have it there colour.models.Lab_to_LCHab BUT it is programmatically generated thus not visible explicitly in the codebase:

for _Jab, _JCh in COLOURSPACE_MODELS_POLAR_CONVERSIONS:

@KelSolaar
Copy link
Member

I will need to add an intermediate definition so that we do not copy-paste the entire CIE 2000 code here.

@KelSolaar
Copy link
Member

KelSolaar commented Nov 10, 2024

Hello,

I added a new colour.difference.delta_e.intermediate_attributes_CIE2000 definition that should have all the required attributes and avoid code duplication.

Let us know how it goes!

Cheers,

Thomas

@mesvam mesvam marked this pull request as ready for review November 10, 2024 05:57
KelSolaar added a commit that referenced this pull request Nov 11, 2024
@KelSolaar KelSolaar changed the base branch from develop to hych_staging November 11, 2024 08:04
@KelSolaar
Copy link
Member

Thanks a lot, I will merge in a staging branch and doctor the history a bit.

@KelSolaar KelSolaar merged commit 59408c0 into colour-science:hych_staging Nov 11, 2024
15 of 16 checks passed
@mesvam
Copy link
Contributor Author

mesvam commented Nov 13, 2024

Also, Equation 6 for CD4 is notated using LCH* in the paper, but I'm seeing L'C'H' used in other sources and also in this package. I'm assuming when the paper uses ΔC*, they mean ΔC' because of the k_C * S_C denominator, but I'm not confident in this. And also since the paper doesn't provide any examples, I can't find any sample data to compare results to verify my implementation.

So I contacted the authors, who kindly provided some test data to compare. I verified that this implementation matched their results to within numerical precision, with the exception of an optional hue rotation term.

You can cherry-pick this mesvam@2730318 if you want to add it to the test suite

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.

2 participants