-
-
Notifications
You must be signed in to change notification settings - Fork 264
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 "MacAdam Limits" computation. #768
base: develop
Are you sure you want to change the base?
PR: Implement support for "MacAdam Limits" computation. #768
Conversation
Hi @gutenzwerg, Thanks, this looks great, initial code perusal tells me that we could optimise some chunks and generalise a bit more but it looks like an awesome start! There are some code style and pedantry things to address. I should have some time in the coming days to look at that properly. Bear with me, I'm super busy! Cheers, Thomas |
Dear Thomas, thanks for Your quick and enjoyable reply. I apologise for my old-fashioned programming-style. It is one of my first projects after a break of about 10 years of programming. On github I am a total newbie. I did single tests with YAPF and flake8 and I got at least no error messages, but trying to commit I got two failure indications. At least I did the upload within the graphical surface of github. So I have rather to ask to bear with me. Do not hurry to much, I am patient. Best regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say anything about the validity of the content, but everything seems straightforward enough, and I don't see anything that jumps out as problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I bit off a bit more than I could chew when I started this review with the time I had available... let me take a closer look at the wording / phrasing this evening.
colour/volume/macadam_limits.py
Outdated
brightness = bright_opti_colour(width, wavelength, Y_illuminated) | ||
if brightness < target_bright: | ||
width += 1 | ||
brightness = bright_opti_colour(width, wavelength, Y_illuminated) | ||
|
||
rough_optimum = optimum_colour(width, wavelength) | ||
brightness = np.sum(rough_optimum * Y_illuminated) / maximum_brightness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brightness = bright_opti_colour(width, wavelength, Y_illuminated) | |
if brightness < target_bright: | |
width += 1 | |
brightness = bright_opti_colour(width, wavelength, Y_illuminated) | |
rough_optimum = optimum_colour(width, wavelength) | |
brightness = np.sum(rough_optimum * Y_illuminated) / maximum_brightness | |
brightness = bright_opti_colour(width, wavelength, Y_illuminated) | |
if brightness < target_bright: | |
width += 1 | |
rough_optimum = optimum_colour(width, wavelength) | |
brightness = np.sum(rough_optimum * Y_illuminated) / maximum_brightness |
Seems like we can get rid of a brightness computation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I tried it out, and it worked perfectly. Thank You again.
As far as I see. My function "macadam_limits" is not build in in the actual version. Am I right? |
Hi @gutenzwerg, Yes, I never had time finish looking at your PR! We should try to rebase it on top of develop. If you are fine with that, I can give it a stab and push back into your branch? Cheers, Thomas |
I created a rebased branch here as an example: https://github.com/colour-science/colour/tree/feature/macadam_limits Looking at the code there are certainly quite a few things that would need to be addressed before we can merge it. |
@KelSolaar,
In the link of Your second post You suggest only one change in one line - that's o.k. If You have other points of critique please tell me. For me the code works fine in the actual state and the great help of zachlewis. I do not know what I should change in colour/init.py, colour/volume/init.py and colour/volume/macadam_limits.py to avoid the error-messages in "This branch has conflicts that must be resolved". best regards, |
This features calculates MacAdam_Limits according to a given brightness for every single whavelenght
This features calculates MacAdam_Limits according to a given brightness for every single whavelenght
This features calculates MacAdam_Limits according to a given brightness for every single whavelenght
A smaller change to support single-whavelenght-sprectra, especially for very dark colours.
There was a blank line with some spaces
Co-authored-by: zachlewis <[email protected]>
ee66aaf
to
afc9946
Compare
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
for more information, see https://pre-commit.ci
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars suggestions
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
for more information, see https://pre-commit.ci
Several updates to meet the recommendations of KelSolaar
Some changes to meet KelSolaars recommendations.
for more information, see https://pre-commit.ci
Some updates to meet KelSolaars recommendations
Some updates to meet KelSolaars recommendations
for more information, see https://pre-commit.ci
Some updates to meet KelSolaars recommendations
An update to meet Kelsolaars recommendations
for more information, see https://pre-commit.ci
I wrote a program to calculate MacAdam-Limits. I presume it is, what was wished in the issue
Implement support for "Optimal Colour Stimuli" computation. #364
Every single whavelenght shoud be accurate enough. If necessary, it is possible to modify the program to use different cmfs instead of only CIE-31 2° by default.