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

Test Image.histogram() for all modes #8394

Closed
wants to merge 4 commits into from

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Sep 19, 2024

And add a hash of the histogram to the values that are checked.

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 19, 2024

Current Errors:
amazon-2-amd64, mode "LAB": assert (768, 0, 1945) == (768, 0, 1946)
debian-12-bookworm-x86, mode "1": assert -470796945 == -6588993366496339844

So it would seem like the histogram calculation is going wrong somehow.

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 19, 2024

I tried changing the histogram C data type from long to UINT32 in case it was an overflow error somehow, but that didn't change anything. https://github.com/Yay295/Pillow/actions/runs/10932277548

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 19, 2024

Mode "1" was failing because the basic hash function isn't deterministic, so I've changed it to use a deterministic hash. I also added the sum to the list of values being checked, and the sum is correct for the failing tests. So all of the values are being counted, but they're not all going in the expected buckets. This makes me think there's actually something going on in the conversion from "RGB" to "LAB" and "HSV" that is somehow not the same in every environment.

@radarhere
Copy link
Member

Rather than introducing the new idea of hashes to our test suite, would it not be simpler to add individual files to check against?

@wiredfool
Copy link
Member

Using hashes seems like a now we have two problems sort of issue. Tests should be clear and obvious, and point to a cause. Comparing to a hash says right or wrong, but doesn’t aid debugging.

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 21, 2024

I used hashes because each mode's histogram data has at least 256 values, and there are 20 modes. That's a lot of numbers to put in a test file. Using hashes allows for a much smaller set of expected data to be stored, and it still solves the purpose of showing when something isn't right.

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 30, 2024

Correct HSV Histogram: [171, 51, 63, 71, 52, 73, 97, 158, 181, 226, 327, 321, 332, 369, 293, 327, 264, 135, 151, 88, 94, 89, 49, 69, 60, 45, 44, 30, 51, 36, 15, 24, 21, 32, 6, 13, 13, 6, 3, 3, 2, 2, 49, 2, 0, 4, 1, 1, 3, 5, 12, 0, 1, 1, 5, 0, 2, 1, 0, 6, 9, 1, 1, 1, 0, 1, 1, 4, 2, 0, 20, 0, 5, 1, 0, 0, 0, 8, 0, 4, 1, 4, 0, 0, 3, 0, 0, 1, 0, 3, 0, 0, 1, 7, 1, 0, 1, 0, 2, 1, 4, 0, 2, 2, 8, 1, 39, 1, 1, 1, 1, 0, 10, 21, 1, 0, 10, 0, 1, 1, 2, 1, 7, 10, 2, 1, 0, 46, 0, 4, 4, 8, 2, 0, 21, 27, 0, 0, 10, 13, 1, 3, 8, 19, 1, 31, 33, 25, 23, 21, 78, 36, 158, 595, 1696, 1140, 262, 174, 150, 100, 48, 114, 183, 72, 110, 59, 134, 156, 134, 861, 52, 148, 121, 85, 81, 197, 278, 592, 412, 49, 30, 62, 63, 14, 68, 14, 17, 236, 130, 8, 14, 37, 34, 9, 0, 62, 21, 14, 168, 15, 110, 5, 7, 10, 4, 108, 9, 48, 24, 4, 6, 2, 144, 0, 4, 16, 34, 7, 2, 18, 11, 157, 13, 0, 39, 29, 58, 2, 24, 11, 13, 97, 56, 79, 6, 34, 11, 10, 9, 39, 155, 12, 14, 14, 112, 16, 48, 53, 21, 29, 100, 43, 77, 48, 31, 0, 37, 25, 36, 32, 21, 54, 48, 49, 18, 26, 38, 35, 41, 27, 40, 52, 34, 46, 36, 44, 37, 38, 34, 29, 31, 28, 34, 21, 37, 27, 101, 178, 21, 34, 186, 24, 81, 21, 35, 39, 39, 22, 66, 33, 14, 45, 28, 62, 14, 54, 20, 107, 30, 46, 57, 35, 49, 43, 47, 22, 60, 42, 17, 125, 23, 34, 34, 102, 46, 65, 166, 29, 96, 21, 26, 161, 80, 41, 36, 116, 93, 53, 57, 22, 12, 218, 27, 70, 50, 45, 114, 71, 62, 90, 62, 66, 61, 59, 68, 48, 87, 17, 117, 119, 59, 70, 77, 102, 59, 100, 110, 72, 119, 99, 144, 204, 223, 284, 222, 354, 223, 165, 95, 123, 180, 77, 92, 176, 74, 70, 99, 101, 129, 90, 128, 86, 120, 100, 83, 105, 102, 107, 79, 112, 85, 105, 100, 119, 117, 98, 137, 161, 84, 157, 127, 141, 168, 203, 144, 164, 129, 113, 99, 95, 74, 85, 87, 69, 82, 31, 114, 86, 49, 79, 57, 78, 72, 48, 54, 49, 91, 61, 68, 65, 63, 57, 38, 67, 45, 52, 39, 56, 45, 47, 41, 40, 34, 44, 38, 37, 32, 35, 43, 16, 42, 31, 21, 26, 22, 22, 35, 25, 21, 22, 14, 16, 21, 20, 23, 15, 18, 26, 16, 17, 14, 19, 16, 18, 15, 23, 13, 15, 21, 14, 18, 17, 16, 19, 14, 18, 13, 18, 4, 6, 12, 9, 10, 8, 4, 6, 1, 1, 1, 3, 0, 121, 0, 1, 0, 0, 0, 3, 1, 6, 5, 6, 14, 31, 38, 75, 147, 289, 350, 348, 332, 294, 223, 174, 212, 163, 209, 232, 199, 211, 167, 190, 160, 168, 147, 154, 130, 88, 90, 76, 80, 71, 62, 59, 69, 71, 64, 59, 59, 49, 64, 59, 44, 50, 59, 48, 45, 45, 52, 55, 53, 43, 37, 51, 48, 45, 48, 37, 56, 54, 51, 52, 50, 45, 42, 44, 31, 45, 53, 40, 34, 45, 39, 39, 43, 43, 30, 39, 36, 22, 26, 25, 27, 22, 19, 25, 23, 18, 19, 12, 7, 15, 8, 18, 14, 20, 12, 19, 15, 5, 9, 15, 15, 9, 18, 16, 19, 19, 15, 19, 17, 15, 17, 21, 16, 25, 26, 25, 25, 31, 24, 15, 21, 18, 17, 11, 26, 27, 19, 25, 25, 21, 28, 27, 24, 25, 23, 19, 20, 23, 22, 29, 32, 24, 31, 26, 35, 22, 33, 29, 18, 24, 29, 25, 34, 32, 30, 33, 26, 34, 37, 37, 35, 42, 49, 45, 54, 62, 110, 167, 178, 166, 116, 119, 102, 112, 108, 117, 131, 111, 140, 133, 106, 94, 91, 119, 105, 114, 142, 93, 121, 182, 218, 308, 295, 188, 182, 204, 154, 155, 88, 86, 84, 72, 67, 64, 52, 59, 42, 44, 50, 31, 47, 42, 33, 42, 31, 49, 44, 54, 42, 30, 35, 38, 35, 33, 26, 35, 37, 27, 24, 25, 27, 26, 24, 31, 27, 29, 33, 26, 36, 33, 18, 29, 35, 38, 27, 316]

Incorrect HSV Histogram: [171, 51, 63, 71, 48, 77, 97, 158, 181, 232, 321, 321, 332, 370, 292, 332, 250, 144, 151, 88, 94, 89, 49, 69, 60, 45, 44, 30, 51, 36, 15, 24, 21, 32, 6, 13, 13, 6, 3, 3, 2, 2, 49, 2, 0, 4, 1, 1, 3, 5, 0, 12, 1, 1, 5, 0, 2, 1, 0, 6, 9, 1, 1, 1, 0, 1, 1, 4, 2, 0, 20, 0, 5, 1, 0, 0, 0, 8, 0, 4, 1, 4, 0, 0, 0, 3, 0, 1, 0, 3, 0, 0, 1, 7, 1, 0, 1, 0, 2, 1, 4, 0, 2, 2, 8, 1, 39, 1, 1, 1, 1, 0, 10, 21, 1, 0, 10, 0, 0, 2, 2, 1, 7, 10, 2, 1, 0, 46, 0, 4, 4, 8, 2, 0, 15, 33, 0, 0, 10, 13, 1, 3, 8, 19, 1, 31, 33, 25, 23, 21, 78, 36, 158, 595, 1696, 1140, 265, 171, 150, 100, 48, 114, 183, 72, 110, 59, 134, 156, 134, 132, 781, 148, 121, 85, 81, 197, 278, 592, 412, 61, 18, 62, 63, 14, 68, 14, 17, 236, 130, 8, 14, 37, 34, 9, 0, 62, 21, 14, 168, 15, 110, 5, 7, 10, 4, 108, 9, 48, 24, 4, 6, 2, 144, 0, 4, 16, 34, 7, 2, 18, 11, 157, 13, 0, 39, 29, 58, 2, 24, 11, 13, 97, 56, 79, 6, 34, 11, 10, 9, 39, 155, 12, 14, 14, 112, 16, 48, 53, 21, 29, 100, 43, 77, 48, 31, 0, 37, 25, 36, 32, 21, 54, 48, 49, 18, 26, 38, 35, 41, 27, 40, 52, 34, 46, 36, 44, 37, 38, 34, 29, 31, 28, 34, 21, 37, 27, 101, 178, 21, 34, 186, 24, 81, 21, 35, 39, 39, 22, 66, 33, 14, 45, 28, 62, 14, 54, 20, 107, 30, 46, 57, 35, 49, 43, 47, 22, 60, 42, 17, 125, 23, 34, 34, 102, 46, 65, 166, 29, 96, 21, 26, 161, 80, 41, 36, 116, 93, 53, 57, 22, 12, 218, 27, 70, 50, 45, 114, 71, 62, 90, 62, 66, 61, 59, 68, 48, 87, 17, 117, 119, 59, 70, 77, 102, 59, 100, 110, 72, 119, 99, 144, 204, 223, 284, 222, 354, 223, 165, 95, 123, 180, 77, 92, 176, 74, 70, 99, 101, 129, 90, 128, 86, 120, 100, 83, 105, 102, 107, 79, 112, 85, 105, 100, 119, 117, 98, 137, 161, 84, 157, 127, 141, 168, 203, 144, 164, 129, 113, 99, 95, 74, 85, 87, 69, 82, 31, 114, 86, 49, 79, 57, 78, 72, 48, 54, 49, 91, 61, 68, 65, 63, 57, 38, 67, 45, 52, 39, 56, 45, 47, 41, 40, 34, 44, 38, 37, 32, 35, 43, 16, 42, 31, 21, 26, 22, 22, 35, 25, 21, 22, 14, 16, 21, 20, 23, 15, 18, 26, 16, 17, 14, 19, 16, 18, 15, 23, 13, 15, 21, 14, 18, 17, 16, 19, 14, 18, 13, 18, 4, 6, 12, 9, 10, 8, 4, 6, 1, 1, 1, 3, 0, 121, 0, 1, 0, 0, 0, 3, 1, 6, 5, 6, 14, 31, 38, 75, 147, 289, 350, 348, 332, 294, 223, 174, 212, 163, 209, 232, 199, 211, 167, 190, 160, 168, 147, 154, 130, 88, 90, 76, 80, 71, 62, 59, 69, 71, 64, 59, 59, 49, 64, 59, 44, 50, 59, 48, 45, 45, 52, 55, 53, 43, 37, 51, 48, 45, 48, 37, 56, 54, 51, 52, 50, 45, 42, 44, 31, 45, 53, 40, 34, 45, 39, 39, 43, 43, 30, 39, 36, 22, 26, 25, 27, 22, 19, 25, 23, 18, 19, 12, 7, 15, 8, 18, 14, 20, 12, 19, 15, 5, 9, 15, 15, 9, 18, 16, 19, 19, 15, 19, 17, 15, 17, 21, 16, 25, 26, 25, 25, 31, 24, 15, 21, 18, 17, 11, 26, 27, 19, 25, 25, 21, 28, 27, 24, 25, 23, 19, 20, 23, 22, 29, 32, 24, 31, 26, 35, 22, 33, 29, 18, 24, 29, 25, 34, 32, 30, 33, 26, 34, 37, 37, 35, 42, 49, 45, 54, 62, 110, 167, 178, 166, 116, 119, 102, 112, 108, 117, 131, 111, 140, 133, 106, 94, 91, 119, 105, 114, 142, 93, 121, 182, 218, 308, 295, 188, 182, 204, 154, 155, 88, 86, 84, 72, 67, 64, 52, 59, 42, 44, 50, 31, 47, 42, 33, 42, 31, 49, 44, 54, 42, 30, 35, 38, 35, 33, 26, 35, 37, 27, 24, 25, 27, 26, 24, 31, 27, 29, 33, 26, 36, 33, 18, 29, 35, 38, 27, 316]

All of the incorrect values are in the "H" part of the histogram. This makes some sense, as the "H" values have the most complicated calculation. It's still not that complicated that I would expect to see differences though.

@hugovk
Copy link
Member

hugovk commented Oct 13, 2024

Using hashes seems like a now we have two problems sort of issue. Tests should be clear and obvious, and point to a cause. Comparing to a hash says right or wrong, but doesn’t aid debugging.

I agree with this.

Thanks for the suggestion, but let's close this one.

@hugovk hugovk closed this Oct 13, 2024
@Yay295
Copy link
Contributor Author

Yay295 commented Oct 13, 2024

Would you prefer it if the entire histogram data was stored in the test file? Or do you just not want to test this code at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants