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

lib_enum.ks allows the partition indices to walk off the ends in the quicksort routine #140

Open
mgalyean opened this issue Dec 7, 2021 · 1 comment

Comments

@mgalyean
Copy link

mgalyean commented Dec 7, 2021

I'm not sure what conditions lead to the i and j indices walking off the ends, but they do sometimes, seemingly randomly but probably not really.
I altered my local copy at line 102 in the sort fn from:
if lo<hi{local p is pt(A, lo, hi). qs(A, lo, p). qs(A, p + 1, hi).}}
to:
if (lo>0 AND hi<A:LENGTH-1) AND lo<hi{local p is pt(A, lo, hi). qs(A, lo, p). qs(A, p + 1, hi).}}
and the issue went away for me. But there is probably a more robust solution

@nuggreat
Copy link
Contributor

nuggreat commented Dec 7, 2021

Mostly just forwarding what we discussed on the kOS discord as well as some other thoughts I have on the subject.

The root cause of the issue from what I could determine likley stems from the chosen compare function as said function was computing the distance along the SHIP:FACING vector for parts. As this is physics linked data should a physics tick fall between getting the data for the two passed in parts it is possible for the same part to have two difference distances. This is a problem for the sort function as written because it assumes that the things being sorted are not volatile and thus when comparing two parts there will be no difference. But should there be a difference and that difference is positive or negative depending that can cause the i and j values in the internal pt function to go out of range.

Once this was determined I was able to construct a reproducible test case with the error by injecting noise into the comparison function. From what I was then able to determine the proposed solution only works in some cases not all. Instead the solution I was able to come up with that worked in all cases reasonable cases I was able to create. The only thing I dislike about the solution is that it comes with an increase in sort time that seams to impact longer lists more. Thus as the issue was narrowed down to being caused by sorting a volatile list I am debating if it should be patched.

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

No branches or pull requests

2 participants