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 "MacAdam Limits" computation. #768

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

Conversation

gutenzwerg
Copy link

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.
MacAdamLimits

@KelSolaar KelSolaar changed the title Feature/macadam limits PR: Implement support for MacAdam Limits computation. Feb 2, 2021
@KelSolaar KelSolaar added this to the v0.4.0 milestone Feb 2, 2021
@KelSolaar
Copy link
Member

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

@gutenzwerg
Copy link
Author

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,
Christian Greim

@KelSolaar KelSolaar changed the title PR: Implement support for MacAdam Limits computation. PR: Implement support for "MacAdam Limits" computation. Feb 4, 2021
Copy link
Contributor

@zachlewis zachlewis left a 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.

colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zachlewis zachlewis left a 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 Show resolved Hide resolved
colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
colour/volume/macadam_limits.py Outdated Show resolved Hide resolved
Comment on lines 258 to 297
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Author

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.

@KelSolaar KelSolaar modified the milestones: v0.4.0, v0.4.1 Feb 19, 2022
@KelSolaar KelSolaar modified the milestones: v0.4.1, v0.4.2 Mar 1, 2022
@gutenzwerg
Copy link
Author

gutenzwerg commented Apr 26, 2022

As far as I see. My function "macadam_limits" is not build in in the actual version. Am I right?
I need help in resolving the moand conflicts written below. It all relates to stuff of rights and features within comment-areas. I have no idea what to write in.
Can anyone help me to resolve the conflicts? Otherwise this feature can not be implemented.

@KelSolaar
Copy link
Member

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

@KelSolaar
Copy link
Member

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.

@gutenzwerg
Copy link
Author

@KelSolaar,
thank You for Your quick response. I am not a native-speaker. I simpliy do not understand some terms in Your first post:

  • PR
  • rebase it on top of develop
  • stab
  • push back into your branch

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,
Christian

gutenzwerg and others added 9 commits May 23, 2022 18:53
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 <zachlewis@users.noreply.github.com>
gutenzwerg and others added 24 commits June 16, 2022 17:21
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
Several updates to meet KelSolaars recommendations
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
Several updates to meet the recommendations of KelSolaar
Some changes to meet KelSolaars recommendations.
Some updates to meet KelSolaars recommendations
Some updates to meet KelSolaars recommendations
Some updates to meet KelSolaars recommendations
An update to meet Kelsolaars recommendations
@KelSolaar KelSolaar modified the milestones: v0.4.2, v0.4.3 Nov 27, 2022
@KelSolaar KelSolaar modified the milestones: v0.4.3, v0.4.4 Aug 25, 2023
@KelSolaar KelSolaar modified the milestones: v0.4.4, v0.4.5 Dec 19, 2023
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

5 participants