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

Fix greyscale computation and inverted coords #8905

Merged
merged 4 commits into from
Jul 29, 2023

Conversation

colinleroy
Copy link
Contributor

Describe your change:

  • Fix a bug or typo in an existing algorithm?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.

The change:

Hello there,

I've used your algorithm to re-implement it in C for a side-project of mine, and noticed two little bugs in it:

  1. the luminance calculation is incorrect - according to Internet, it's either (0.2126R + 0.7152G + 0.0722B) or (0.299R + 0.587G + 0.114B). Here, we had a mix of both (and the greyscale value could never reach 255.

  2. the current_error setting used inverted coordinates.

Hope this helps!

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Jul 29, 2023
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed and removed tests are failing Do not merge until tests pass labels Jul 29, 2023
Copy link
Contributor

@CaedenPH CaedenPH left a comment

Choose a reason for hiding this comment

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

Could you add some doctests to prove that it will reach 255 and works correctly please

@colinleroy
Copy link
Contributor Author

Could you add some doctests to prove that it will reach 255 and works correctly please

Good idea, thanks. done :)

Copy link
Contributor

@CaedenPH CaedenPH left a comment

Choose a reason for hiding this comment

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

LGTM!

"""
return 0.114 * blue + 0.587 * green + 0.2126 * red
return 0.114 * blue + 0.587 * green + 0.299 * red
Copy link
Contributor

@tianyizheng02 tianyizheng02 Jul 29, 2023

Choose a reason for hiding this comment

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

according to Internet, it's either (0.2126R + 0.7152G + 0.0722B) or (0.299R + 0.587G + 0.114B)

Could you add a source for these numbers so that we can avoid mistakes like this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done :)

@tianyizheng02 tianyizheng02 merged commit 2cfef09 into TheAlgorithms:master Jul 29, 2023
@algorithms-keeper algorithms-keeper bot removed the awaiting reviews This PR is ready to be reviewed label Jul 29, 2023
sedatguzelsemme pushed a commit to sedatguzelsemme/Python that referenced this pull request Sep 15, 2024
* Fix greyscale computation and inverted coords

* Fix test

* Add test cases

* Add reference to the greyscaling formula

---------

Co-authored-by: Colin Leroy-Mira <colin.leroy-mira@sigfox.com>
@isidroas isidroas mentioned this pull request Jan 25, 2025
14 tasks
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.

3 participants