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

sort: document the ordering requirements of Less #34915

Closed
jowu opened this issue Oct 15, 2019 · 11 comments
Closed

sort: document the ordering requirements of Less #34915

jowu opened this issue Oct 15, 2019 · 11 comments
Labels
Documentation FrozenDueToAge help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@jowu
Copy link

jowu commented Oct 15, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13rc1 linux/amd64

But this should apply to all.

Does this issue reproduce with the latest release?

Yes.

sort doc should remind users that the 'less' function must be a total order.

@agnivade
Copy link
Contributor

Hi @jowu - could you please explain what you mean by "total order" ? Thanks.

@jowu
Copy link
Author

jowu commented Oct 15, 2019

Sure. In short, the function has to meet these criteria: https://en.wikipedia.org/wiki/Total_order
Once you think about sorting, this is fairly obvious, but when you are working on something else, it's easy to forget (a colleague and I wasted a lot of time trying to find bugs elsewhere because we'd forgotten this basic property).

@ALTree ALTree changed the title [docs] sort doc should remind users that the 'less' function must be a total order sort: documentation should remind users that the 'less' function must be a total order Oct 15, 2019
@randall77
Copy link
Contributor

The less function does not need to be a total order. If it were, then there would be no difference between sort.Sort and sort.Stable. It is allowed that both Less(a,b) and Less(b,a) return false.

Less does need to be transitive.

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Oct 15, 2019
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Oct 15, 2019
@randall77
Copy link
Contributor

I think the correct term is strict partial order.

@smasher164
Copy link
Member

That being said, the incomparability of Less is transitive, where incomparability occurs when Less(a,b) and Less(b,a) return false.

So if Less(a,b) and Less(b,a) and Less(b,c) and Less(c,b) all return false, then Less(a,c) and Less(c,a) return false as well.

A specific term for this type of strict partial order is a strict weak ordering.

@randall77
Copy link
Contributor

So if Less(a,b) and Less(b,a) and Less(b,c) and Less(c,b) all return false, then Less(a,c) and Less(c,a) return false as well.

I don't think that is right. You can have b be unordered with respect to both a and c and still have a<c.

@smasher164
Copy link
Member

You can have b be unordered with respect to both a and c and still have a<c.

Doesn't that imply that there's no way to distinguish between two elements being equal and two elements being incomparable? Additionally, I don't think sort reorders the list [c, b, a] if b is unordered wrt a and c, but a<c.

@randall77
Copy link
Contributor

Additionally, I don't think sort reorders the list [c, b, a] if b is unordered wrt a and c, but a<c.

Indeed, this is strange. I guess it is expected now that I think about it, quicksort won't work if a completely incomparable element is picked as the pivot. So maybe there is something stronger we require. Less must divide the input set into equivalence classes, where two elements are in the same equivalence class if !Less(a,b) && !Less(b,a), and the equivalence classes must be totally ordered? That certainly sounds like the strict weak ordering you mentioned.

package main

import "fmt"
import "sort"

type S string

func f(a []S) {
	sort.Slice(a, func(i, j int) bool {
		if a[i] == "a" && a[j] == "c" {
			return true
		}
		return false
	})
	fmt.Printf("%v\n", a)
}

func main() {
	f([]S{"c", "b", "a"})
	f([]S{"c", "a", "b"})
}

prints

[c b a]
[a c b]

@ianlancetaylor
Copy link
Contributor

@smasher164

Doesn't that imply that there's no way to distinguish between two elements being equal and two elements being incomparable?

What does it mean to sort a list of values when some of the values are incomparable? What do you expect to happen?

@smasher164
Copy link
Member

@randall77

Less must divide the input set into equivalence classes, where two elements are in the same equivalence class if !Less(a,b) && !Less(b,a), and the equivalence classes must be totally ordered?

Yes, that is exactly the equivalence relation we want. We want a totally-ordered partition where Less(a,b) implies either Less(a,c) OR Less(c,b).

@ianlancetaylor

What does it mean to sort a list of values when some of the values are incomparable? What do you expect to happen?

If incomparability is transitive, then I should expect incomparable values to be grouped together. However, if that contract is violated like in Keith's example, then the sort may not be accurate.

@smasher164 smasher164 changed the title sort: documentation should remind users that the 'less' function must be a total order sort: document the ordering requirements of Less Nov 6, 2019
@smasher164 smasher164 added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Nov 19, 2019
@gopherbot
Copy link

Change https://golang.org/cl/261959 mentions this issue: sort: document requirements of Less relation

@golang golang locked and limited conversation to collaborators Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants