-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Avoid unstable nature of qsort in Quant.c #5367
Conversation
I have looked over the code changes to A small quibble: the Shell sort in #5264 is not stable. The object was to get the same sort order results on any platform, and incorporating the sort in The Shell sort is only a little slower than Quicksort until the array gets fairly large (and can be faster, e.g. if the data are already nearly in order). Generally the compare function contributes heavily, so a more complex compare (as here) can slow things down. I have not tried to profile or time these two approaches so cannot say which will be faster in practice. I'm the new guy here and @radarhere is a longtime member, so I feel I have to tread lightly. But I am curious as to the motivation. This seems to add more complexity than just using an embedded sort, and you've gone to rather a lot of effort to do it. Is there a concern about the performance, or maybe about the correctness of my Shell sort? This approach is a sort of use of the decorate-sort-undecorate idiom. You need to allocate a new array of structs. I don't know how much memory this might take in the worst case, but based on the But I see that there are only I also don't know if the added time to "decorate" and "undecorate" the The name Your construction of the You've not made I'm sure you'll choose one or the other of these approaches, and I am fine with either, but if you go with keeping the system |
I have definitely not thought this through, but would this suffice? Build for (i = 0; i < nEntries; i++) {
for (j = 0; j < nEntries; j++) {
dwi[j] = (DistanceWithIndex){
avgDistSortKey[i * nEntries + j],
j
};
} // ; -- removed superfluous semicolon! rdg 20210402
qsort(
dwi,
nEntries,
sizeof(DistanceWithIndex),
_sort_ulong_ptr_keys);
for (j = 0; j < nEntries; j++) {
avgDistSortKey[i * nEntries + j] = dwi[j].distance;
}
} Only need |
I've created #5374, to separate out that change from this discussion. |
My hope with this PR is that is adds less complexity - the nuances of a sort function seem more complicated to me than temporarily replacing a single variable with a struct. If you count it up, fewer lines are modified. And if our compare function is leaving room for interpretation, then I'd rather clarify that to address what I think of as the root of the problem. I can appreciate that your opinion is probably that inserting a custom sort function is simpler from a big picture perspective. I guess my other concern is that we're potentially re-inventing the wheel by doing so. Don't feel you have to tread lightly (I mean, politeness is still good... you know what I mean). I don't have strong opinions here. If your PR is merged instead, then onto the next issue. You make a good point about the memory allocation. I've pushed a commit for that and to rename the function. |
@radarhere With your change to reduce memory (which reduces complexity as well), I have withdrawn my PR. Your newer version generates exactly the same results as my similar suggestion above; I tested. So I feel good about this version. I do get your concerns about the sort function. My Shell sort was adapted for use as |
@radarhere , I would like to see this get merged in the upcoming release. Then I will not have to make my own Windows build anymore. Are there any blockers keeping this from getting merged? |
Just a case of waiting for someone else from the Pillow team to review this. |
Are there any discernible performance impacts from this, for example resizing large images? |
Using https://photojournal.jpl.nasa.gov/jpeg/PIA24472.jpg (large enough that it triggers a DecompressionBombWarning), and import timeit
from PIL import Image
def test():
im = Image.open("PIA24472.jpg")
im.resize((int(im.width/2), int(im.height/2)))
im.close()
print(timeit.timeit(test, number=100)) With this PR, I get 108.989, 108.575 and 108.075 So no obvious differences in terms of speed. |
How about resizing to something small, say |
On master, 62.347, 61.433 and 61.384. So still no obvious difference. |
Thanks for checking! |
Resolves #5263
https://en.wikipedia.org/wiki/Quicksort
In other words, if
qsort
is given two equal items to sort, it doesn't have to return them in the same order on one platform vs another. This has lead to #5263.#5264 solves this by adding a custom stable sort instead of
qsort
.This PR is an alternative (and feel free to favour #5264 instead if you so choose). It instead keeps
qsort
but ensures that no two items are equal, by adding an index to the items, so that even if one dimension is equal, the second one will not be. It also reverts #5363 in the process.Both PRs have the same code added to the test suite.