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

Implement simulators for dichromacy and anomalous trichromacy #414

Closed
wants to merge 9 commits into from

Conversation

Kirk-Fox
Copy link
Contributor

This pull request implements the Brettel 1997 and Vienot 1999 approaches to simulating color vision deficiency as described here. For anomalous trichromacy, I still am trying to understand the Machado 2009 method, but was unable to implement it thus far. It is still possible to include it at a later date and institute it as the new default for protanomaly and deuteranomaly, due to its handling of anomalous trichromacy being much more principled than using simple linear interpolation.

I am not sure of the best way to implement tests for these new features and any help in that respect would be appreciated.

Copy link

codspeed-hq bot commented Aug 13, 2024

CodSpeed Performance Report

Merging #414 will improve performances by 20.29%

Comparing Kirk-Fox:cvd (e860db6) with master (fab4412)

Summary

⚡ 1 improvements
✅ 46 untouched benchmarks

Benchmarks breakdown

Benchmark master Kirk-Fox:cvd Change
multiply_3x3 345.8 ns 287.5 ns +20.29%

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 242 lines in your changes missing coverage. Please review.

Project coverage is 81.81%. Comparing base (fab4412) to head (f877cbd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
- Coverage   82.80%   81.81%   -0.99%     
==========================================
  Files         130      132       +2     
  Lines       20039    20281     +242     
  Branches    20039    20281     +242     
==========================================
  Hits        16593    16593              
- Misses       3319     3561     +242     
  Partials      127      127              
Components Coverage Δ
palette 81.81% <0.00%> (-1.05%) ⬇️
palette_derive 81.98% <ø> (ø)

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

Thank you for looking into this and I appreciate the effort, but I'm still not sure it belongs in this library. It's one of those adjacent fields that I hesitate to include if they have a risk of growing too much. I'm not sure I can maintain it on my own, so it would have to be fine for it to stagnate without a "champion". It wouldn't be fair of me to merge it if I can't keep it going. That's why I phrased the issue as more of a research task, though I admit it's sparsely phrased.

I would be open to something relatively small, that's "write once, use forever". Say, something like the chromatic_adaptation, color_difference or color_theory modules, that very rarely need to grow and barely require maintenance. It could be a set of functions that generate convert::Matrix3 matrices, for example. Otherwise, my standpoint from #352 hasn't changed much.

What's your point of view, so far?

@Kirk-Fox
Copy link
Contributor Author

I think that's a fair point. In the process of implementing this I was having a lot of trouble figuring out how to get it to work with the ecosystem of palette and I feel like it is unique enough from the goals of palette that it would deserve its own crate. I more was interested in following this for my own purposes due to trying to implement color deficient modes in a project I was using palette for.

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

Yeah, it's probably something that needs to be experimented with a bit. There seem to be multiple parts to it that are a bit loosely defined in Palette (especially different CIE XYZ versions), so it may be better to start identifying them first and build up to better support. Lms and Matrix3 are a start, at least.

I can't promise it will be integrated, but don't let me stop you from continuing for your own project. A clear use case always helps! If you find any problems along the way, we can tackle them separately and hopefully make things easier. That's more of a bottom-up approach that may be easier than trying to get it in as one unit.

Does that sound good?

@Kirk-Fox
Copy link
Contributor Author

Yeah, although there are other things higher on my list than more into this, so it might be a bit before I continue on it.

@Ogeon
Copy link
Owner

Ogeon commented Aug 18, 2024

You should of course do what you feel like. 👍 I have no expectations. 🙂 I'll close this one for now.

@Ogeon Ogeon closed this Aug 18, 2024
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.

CVD simulation
2 participants