Quick sort and related Arrays utilities #58

prvshah51 · 2022-10-21T20:39:04Z

Fix #54
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

The latter doesn’t work well in CI here because the /verificationLogger options cause extra, unpredictable output lines

robin-aws · 2022-11-14T18:39:13Z

src/Comparison.dfy

+      cmp(t0, t1)
+    }
+
+     predicate Complete??(t0: T, t1: T) {


I'm not a big fan of the ?? suffix. We may want to engage in some bikeshedding about style here before we introduce it. Based on my experience even Valid? below is not common, Valid is much more common and is even baked into the language through {:autocontracts}.

I'm not necessarily to using ? in user-defined constructs, I'd just like to agree on concrete guidance in the style guide on it. :)

Okay I see where the idea came from at least - you almost want to overload Complete? to accept either a pair or a set.

Could this be CompleteOnPair perhaps?

Yeah, the ?? was due to have two layers. Happy with calling them Complete and Complete' (the OCaml style) or Complete1 (the Lisp style) or even Complete_ or Complete2. CompletePair is long.

Okay, let's go with removing all of the single ? suffixes (except for Le? and Ge? above), and replacing all of the double ?? suffixes with a ' instead.

So in this case Complete?? will become Complete', and Complete? will become Complete.

robin-aws

So naturally I've focussed most of my reviewing capacity on arguing about question marks. :) I do actually think it's a really important point for readability though, so I'd love to nail that down first before I look more deeply at the proofs.

src/Collections/Sets/Sets.dfy

src/Collections/Arrays/Arrays.dfy

robin-aws · 2022-11-16T17:31:14Z

src/Collections/Arrays/Arrays.dfy

+      reads arr
+      ensures t in arr[lo..hi]
+    {
+      arr[lo] // TODO


We should either implement this or drop the function and just inline this choice of pivot above.

Let's implement it — it's not hard and it's an important part of making the algorithm safe.

As per offline conversation we decided not to block on this, but in that case we should add a comment on QuickSort that it doesn't yet have the intended O(n log n) worst case runtime yet (which is what @cpitclaudel was referring to as "safe").

robin-aws · 2022-11-16T17:33:08Z

src/Collections/Arrays/Arrays.dfy

+      }
+    }
+
+    lemma Sortable_Slice(arr: array<T>, lo: int, hi: int, lo': int, hi': int)


Suggested change

lemma Sortable_Slice(arr: array<T>, lo: int, hi: int, lo': int, hi': int)

lemma SortableSlice(arr: array<T>, lo: int, hi: int, lo': int, hi': int)

Only instance of an underscore I see here and it really sticks out.

Also consider SortableSubslice instead.

SortableSlice or SortableSubslice both makes sense putting Subslice describes it better.

robin-aws · 2022-11-16T17:37:32Z

src/Collections/Arrays/Arrays.dfy

+import opened Comparison
+import opened Wrappers
+
+  trait Sorter<T> {


This definitely needs an example in examples to demonstrate using it. I assume you have to not only provide a concrete class that implements Sorter<T> and fills in Cmp, but perhaps even has to invoke a lemma or two from the trait in order to prove that it works.

Is it possible to also provide a PredicateSorter class that is constructed around a (T, T) -> Cmp function value? Perhaps it could even be a partial function if you also provide a ghost set of all values you intend to sort with it.

Just to clarify, I'd strongly prefer to have an example, but the PredicateSorter idea can wait for a future PR.

Oh yikes, I went to write an example just to play around with it and immediately got the A class may only extend a trait in the same module, unless the parent trait is annotated with {:termination false} error. That means this is currently useless unless we add {:termination false}.

Unfortunately we should treat that as a hard blocker given there's already been resistance to putting {:termination false} on any standard library code: #22 And especially given the discovery of dafny-lang/dafny#2500 I'm not inclined to push back on that.

We can think about whether there's a good alternative for this code that doesn't use traits, but it might be better to stick with this if it's good UX and at least wait for the solution to dafny-lang/dafny#2500.

That's not right: the proper API to use is PredicateSorter, which is a regular class, defined right below Sorter. That doesn't require :termination false.

Original code here, with examples: https://github.com/dafny-lang/compiler-bootstrap/blob/4f616822209828e48cf63d3da66ee1c010f689d4/src/Utils/Lib.Sort.dfy#L396-L420

Thanks @cpitclaudel, I see the issue now is this PR doesn't include the PredicateSorter class. We should just add that as well.

src/Comparison.dfy

robin-aws · 2022-11-16T17:42:58Z

src/Comparison.dfy

+    const Le? := this != Gt
+    const Ge? := this != Lt


I definitely like this use of ? at least, since it's a very natural extension of the built-in Lt?/Eq?/Gt? discriminator syntax.

robin-aws · 2022-11-16T17:48:35Z

src/Comparison.dfy

+      Complete?(ts) && /* Antisymmetric?(ts) && */ Transitive?(ts)
+    }
+
+    predicate Sorted(sq: seq<T>) {


Why wouldn't this have a ? as well then?

I'll be happiest if we come up with a simple rule for where ? belongs. I definitely don't think it should be every predicate, especially since changing Valid to Valid? will be very disruptive for arguably not a lot of benefit. It looks like the pattern here is if the predicate is "about" the receiver rather than the arguments, but that feels very fuzzy to me, and I don't find it improves readability personally.

Perhaps to start, the straw-person proposal could be to only use it in symbols that aren't used like function calls, such as the Le? and Ge? constants above.

See my other comment.

robin-aws · 2022-11-16T17:52:10Z

src/Comparison.dfy

+      cmp(t0, t1)
+    }
+
+     predicate Complete??(t0: T, t1: T) {


Okay I see where the idea came from at least - you almost want to overload Complete? to accept either a pair or a set.

Could this be CompleteOnPair perhaps?

prvshah51 · 2022-11-17T22:41:56Z

src/Collections/Arrays/Arrays.dfy

+      requires 0 <= lo <= hi <= arr.Length
+      reads arr
+    {
+      multiset(arr[lo..hi]) == old(multiset(arr[lo..hi]))


I remember adding this to almost every every function method in MergeSort.dfy will need to replace that with shuffled instead.

Co-authored-by: Robin Salkeld <salkeldr@amazon.com>

robin-aws · 2022-12-02T04:00:49Z

src/Collections/Arrays/Arrays.dfy

+import opened Comparison
+import opened Wrappers
+
+  trait Sorter<T> {


Just to clarify, I'd strongly prefer to have an example, but the PredicateSorter idea can wait for a future PR.

robin-aws · 2022-12-02T04:09:41Z

src/Comparison.dfy

+      cmp(t0, t1)
+    }
+
+     predicate Complete??(t0: T, t1: T) {


Okay, let's go with removing all of the single ? suffixes (except for Le? and Ge? above), and replacing all of the double ?? suffixes with a ' instead.

So in this case Complete?? will become Complete', and Complete? will become Complete.

robin-aws · 2022-12-02T04:10:34Z

src/Comparison.dfy

+    {}
+
+    }
+   }


Suggested change

}

}

robin-aws · 2022-12-02T04:14:27Z

src/Comparison.dfy

+    }
+
+    predicate {:opaque} Valid?(ts: set<T>) {
+      Complete?(ts) && /* Antisymmetric?(ts) && */ Transitive?(ts)


Suggested change

Complete?(ts) && /* Antisymmetric?(ts) && */ Transitive?(ts)

Complete?(ts) && */ Transitive?(ts)

Just following the general rule of not checking in commented out code. We could put a comment saying something like "we want to include Antisymmetric(ts) as well but it isn't necessary and makes verification timeout" (for example, I don't actually know in this case).

robin-aws · 2022-12-02T04:31:15Z

src/Comparison.dfy

+    }
+
+     predicate Complete??(t0: T, t1: T) {
+      cmp(t0, t1) == cmp(t1, t0).Flip()


It threw me a bit that I didn't recognize this isn't one of the standard properties of total orderings, until I realized that "Transitive" really meant "cmp(...).Le? is a transitive binary relation" and similarly for "Reflexive". Is "complete" a standard term for this property of this kind of comparator?

I expect at the point where we try to connect Relations to this version, we'll have a lemma that proves that C.Valid(s) ==> Relations.TotalOrdering((t1, t2) => C.cmp(t1, t2).Le?)

robin-aws · 2022-12-02T04:33:02Z

src/Collections/Arrays/Arrays.dfy

+      ensures Shuffled(arr, lo, hi)
+    {}
+
+    twostate predicate SameElements(arr: array<T>, lo: int, hi: int)


Consider UnchangedExceptSliceShuffled instead.

robin-aws · 2022-12-02T04:38:33Z

src/Collections/Arrays/Arrays.dfy

+      reads arr
+      ensures t in arr[lo..hi]
+    {
+      arr[lo] // TODO


As per offline conversation we decided not to block on this, but in that case we should add a comment on QuickSort that it doesn't yet have the intended O(n log n) worst case runtime yet (which is what @cpitclaudel was referring to as "safe").

robin-aws · 2022-12-02T04:39:57Z

src/Comparison.dfy

+      Complete?(ts) && /* Antisymmetric?(ts) && */ Transitive?(ts)
+    }
+
+    predicate Sorted(sq: seq<T>) {


See my other comment.

…aries into QuickSort-prvshah

robin-aws

I'm afraid I've realized this won't work as is and can't be made to work safely with current Dafny limitations - see my ~~first~~ last comment. We'll have to pause on this for a while.

robin-aws · 2022-12-02T21:48:23Z

src/Comparison.dfy

+      cmp(t0, t1)
+    }
+
+     predicate CompleteonPair(t0: T, t1: T) {


Suggested change

predicate CompleteonPair(t0: T, t1: T) {

predicate Complete'(t0: T, t1: T) {

CompleteonPair was my earlier not-as-good suggestion, let's do Complete' instead :)

robin-aws · 2022-12-02T22:03:41Z

src/Collections/Arrays/Arrays.dfy

+
+    function method Cmp(t0: T, t1: T): Cmp
+
+    twostate predicate UnchangedSlice(arr: array<T>, lo: int, hi: int)


Just food for thought, or a potential learning exercise: It might make all of this more readable if we had an actual ArraySlice datatype to pass around containing these values. It's a great example of the classic Design Pattern of introducing a type to hold collections of values that are always passed around together. Something like:

datatype ArraySlice_<T> = ArraySlice_(arr: array<T>, lo: int, hi: int) { predicate Valid() { 0 <= lo <= hi <= arr.Length } twostate predicate Unchanged() requires Valid() reads arr { arr[lo..hi] == old(arr[lo..hi]) } // etc. } type ArraySlice<T> = a: ArraySlice_<T> | a.Valid() witness *

AFAICT this would be useful even if we only used it in ghost code to avoid any runtime cost (and even then it's possible we could optimize the wrapper away in the future, just as we now do for single-field datatypes).

robin-aws · 2022-12-02T22:14:36Z

src/Collections/Arrays/Arrays.dfy

+import opened Comparison
+import opened Wrappers
+
+  trait Sorter<T> {


Oh yikes, I went to write an example just to play around with it and immediately got the A class may only extend a trait in the same module, unless the parent trait is annotated with {:termination false} error. That means this is currently useless unless we add {:termination false}.

Unfortunately we should treat that as a hard blocker given there's already been resistance to putting {:termination false} on any standard library code: #22 And especially given the discovery of dafny-lang/dafny#2500 I'm not inclined to push back on that.

We can think about whether there's a good alternative for this code that doesn't use traits, but it might be better to stick with this if it's good UX and at least wait for the solution to dafny-lang/dafny#2500.

Parva Shah and others added 27 commits October 21, 2022 16:34

adding Heap Sort and Bianry Search

2e62624

changes as suggestions

00e8667

adding dependencies

fd12d32

adding 'RUN' line

d8f8cee

'RUN' line

57e35ae

chnages as suggested for verification

ac104b2

removing predicates and lemmas not needed,formatting.

9af98ed

adding copyrights info.

aab9de9

more changes as suggested to remove unused import and formatting.

513d165

following suggestions

f44ea6f

changes file structure to make import and exports simple.

71fe474

only verification error is compare expecting bool and not CompResult.

41faf37

more changes , updating local changes.

33a43a5

forgot to add before.

5c8f06f

Delete Lexicographics.dfy

7da871a

Delete LexicographicHelpers.dfy

62b9346

suggested changes , removed Lexicographics and LexicographicsHelper .

e8e5919

removed unnecessary exports, includes from more files.

bb009fb

Remove Comparison.dfy and revert to plain relations, general cleanup

6d57e55

Move Relations.dfy to src (ala Functions.dfy)

9038f2a

Making example verification cheaper, fixing Wrappers.dfy lit test

a2edede

Use OutputCheck instead of *.expect files

1df340f

The latter doesn’t work well in CI here because the /verificationLogger options cause extra, unpredictable output lines

Revert remaining unneeded edits

850e3d4

Newline

34e380a

Start to use /functionSyntax:4, don’t encourage compiling SortedBy

730278a

Revert adding /functionSyntax:4

f1f31ad

Adding Quick Sort and Arrays related utilities

92a19bd

prvshah51 changed the base branch from master to cpitclaudel_triggers October 21, 2022 20:39

prvshah51 changed the base branch from cpitclaudel_triggers to master October 21, 2022 20:39

edit to run tests again

bc018cb

prvshah51 requested a review from cpitclaudel November 14, 2022 14:42

robin-aws reviewed Nov 14, 2022

View reviewed changes

prvshah51 added 2 commits November 14, 2022 15:25

using different synrtax for CI

ae832a4

for another file

a508b99

robin-aws requested changes Nov 16, 2022

View reviewed changes

prvshah51 commented Nov 17, 2022

View reviewed changes

Apply suggestions from review for readability

03af996

Co-authored-by: Robin Salkeld <salkeldr@amazon.com>

prvshah51 self-assigned this Nov 29, 2022

robin-aws reviewed Dec 2, 2022

View reviewed changes

prvshah51 added 2 commits December 2, 2022 16:38

improving readabilty as per suggestions

467f587

Merge branch 'QuickSort-prvshah' of https://github.com/prvshah51/libr…

9688843

…aries into QuickSort-prvshah

prvshah51 requested a review from robin-aws December 2, 2022 21:45

robin-aws requested changes Dec 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick sort and related Arrays utilities #58

Quick sort and related Arrays utilities #58

prvshah51 commented Oct 21, 2022

robin-aws Nov 14, 2022

robin-aws Nov 16, 2022

cpitclaudel Nov 18, 2022

robin-aws Dec 2, 2022

robin-aws left a comment

robin-aws Nov 16, 2022

cpitclaudel Nov 18, 2022

robin-aws Dec 2, 2022

robin-aws Nov 16, 2022

prvshah51 Dec 2, 2022

robin-aws Nov 16, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

cpitclaudel Dec 5, 2022

robin-aws Dec 5, 2022

robin-aws Nov 16, 2022

robin-aws Nov 16, 2022

robin-aws Dec 2, 2022

robin-aws Nov 16, 2022

prvshah51 Nov 17, 2022 •

edited

Loading

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws left a comment •

edited

Loading

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

robin-aws Dec 2, 2022

	lemma Sortable_Slice(arr: array<T>, lo: int, hi: int, lo': int, hi': int)
	lemma SortableSlice(arr: array<T>, lo: int, hi: int, lo': int, hi': int)

+                  {}
+                  }
+                 }

	Complete?(ts) && /* Antisymmetric?(ts) && */ Transitive?(ts)
	Complete?(ts) && */ Transitive?(ts)

	predicate CompleteonPair(t0: T, t1: T) {
	predicate Complete'(t0: T, t1: T) {


		function method Cmp(t0: T, t1: T): Cmp

		twostate predicate UnchangedSlice(arr: array<T>, lo: int, hi: int)

Quick sort and related Arrays utilities #58

Are you sure you want to change the base?

Quick sort and related Arrays utilities #58

Conversation

prvshah51 commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prvshah51 Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robin-aws left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prvshah51 Nov 17, 2022 •

edited

Loading

robin-aws left a comment •

edited

Loading