brownplt / pyret-lang

The Pyret language.
Other
1.07k stars 110 forks source link

Stable sorting of tables #1695

Closed schanzer closed 4 months ago

schanzer commented 1 year ago

Bootstrap:DS switched from method syntax to function syntax, and we're already seeing huge gains from teachers. Coupled with the Circles of Evaluation, they are immediately more comfortable composing operations. One of the most common cases is composing sorting operations, for example "sort animals alphabetically by species, then by age".

Our order-by method makes no guarantees about stability, of course, so teachers have been consistently surprised by the results!

What's fascinating is that this hasn't been an issue for the past 7 years with method syntax -- even though it's the exact same behavior. That suggests that methods were either hard enough that teachers weren't composing them, or that they were mysterious enough that teachers didn't have any confidence their own expectations of how they should work.

Anyway, this has now bubbled up to the point where it's become a sticking point, so I'm wondering what our options are here. Pyret appears to walk the table from the bottom up when sorting, presumably for performance/memory reasons. Is that consistent enough that I could invert the process in the function that wraps the call to .order-by? Or would this require a change in the runtime itself?

blerner commented 1 year ago

Under the hood, tables convert themselves to lists, and then use lists' sort-by method, which uses a quicksort-like algorithm...which is written to be exactly unstable when it comes to ties.

There are several options here, and I'm curious about @jpolitz and @shriram 's opinions here. We don't have a general-purpose sorting function on raw arrays (only on arrays of numbers), so converting to an array and using JS's sorting isn't going to suffice. We could rewrite the existing sort-by, to make the ties be handled in stable order. We could add a stable-sort-by method. Or we could add a method to tables to flip them upside down, and then you could call the existing order-by method.

I dunno which of these is "best"...

shriram commented 1 year ago

I feel like, as a "teaching" language, we should

Instability is a bit like floats: it's useful if you want to make a point, and it's inevitable beyond some threshold, but when you don't want to make that point, you'd really rather not see it show up at all.

So could we switch to stability by default (mergesort, say) for lists also? And then inherit that for tables?

schanzer commented 1 year ago

Oh my goodness yes, the float analogy resonates SO much for me.

blerner commented 1 year ago

Ok. Sounds like an easy first issue for a student to work on?

shriram commented 1 year ago

I see Luke isn't on this repo. Is this really a CPO thing? Isn't this more a pyret-lang thing?

blerner commented 1 year ago

It is; I'll transfer the issue.

shriram commented 1 year ago

Thanks. Please tag Luke West there.

schanzer commented 1 year ago

Major kudos to @blerner for jumping in with a quick patch. It's now live on the web! I'll remove it when this issue is closed.

schanzer commented 1 year ago

Tagging @westluke as per Shriram's request

blerner commented 5 months ago

No motion on this in a year, so I wrote up a merge-sort implementation for lists. Note that I did not replace the existing implementation of sort and sort-by, but instead added stable-sort and stable-sort-by (in both method and function form). I then revised tables to use stable-sort-by instead. This seems to me the minimally-invasive approach that doesn't break any existing behavior. If we choose to change the stability behavior of sort itself, that is a breaking change and we should decide on it soon while we're first creating essentials2024...

(BTW: Also note, @jpolitz and @shriram , that it's really ugly to write an imperative while-loop-based algorithm in Pyret :( I got as close as I could to a named-let (let loop ...) construct, and I kinda hate it (even more than I dislike named-let ;-) ) I could not think of any way to kludge a Pyret for loop into a syntactically nice while-loop.)

schanzer commented 5 months ago

Thanks for moving this forward, @blerner !! Personally I can't imagine any code that depends on unstable sort, but I'll defer to wiser heads for this decision.

schanzer commented 4 months ago

@jpolitz confirming this isn't supposed to be on CPO yet, right?

blerner commented 4 months ago

No, it's on horizon so far.