Add AA Tree and tests by hummy123 · Pull Request #203 · fsprojects/FSharpx.Collections

hummy123 · 2022-12-12T10:45:30Z

I wasn't able to run the test in this repository as I don't have .NET 6.0.401 SDK installed (only a version matching .NET 7), but I don't expect anything to break since it was just mostly a copy-paste from my other repository.

Appreciate the time taken in reviewing this.

gdziadkiewicz · 2022-12-13T18:44:17Z

Checking.

gdziadkiewicz · 2022-12-13T18:54:11Z

src/FSharpx.Collections.Experimental/AaTree.fs

+/// A balanced binary tree similar to a red-black tree which may have less predictable performance.
+type AaTree<'T when 'T: comparison> = 
+    | E
+    | T of int * AaTree<'T> * 'T * AaTree<'T>


Consider using names for the T case data as in the Shape example here - https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/discriminated-unions .

Good idea. Would be helpful when editing the tree itself to specify the different data types mean. (Added.)

…constructor

gdziadkiewicz · 2022-12-18T11:52:18Z

tests/FSharpx.Collections.Experimental.Tests/AaTreeTest.fs

+        testList "AaTree" [
+
+        (* Existence tests. *)
+        test "test isEmpty" {


Can we divide the tests to have test per assertion/aspect and explain in the test name which aspect of isEmpty we are testing? Here I would go with something along the lines of "test isEmpty returns true for an empty aatree" and "test isEmpty returns false for a single element aatree". This is additional work for you to do now, but it will make future maintenance easier.

gdziadkiewicz · 2022-12-18T11:53:09Z

tests/FSharpx.Collections.Experimental.Tests/AaTreeTest.fs

+
+        (* Existence tests. *)
+        test "test isEmpty" {
+            Expect.isTrue <| AaTree.isEmpty AaTree.empty <| ""


Let's not use empty assertion messages

gdziadkiewicz · 2022-12-18T12:03:08Z

tests/FSharpx.Collections.Experimental.Tests/AaTreeTest.fs

+
+        test "test tryFind" {
+            let tree = AaTree.ofList ["hello"; "bye"]
+            Expect.equal (Some("hello")) <| AaTree.tryFind "hello" tree <| ""


Consider using AAA(arrange, act, assert) structure for the test.
Maybe:

let tree = AaTree.ofList ["hello"; "bye"] let result = AaTree.tryFind "hello" tree Expect.equal (Some "hello" ) result "tryFind should find hello in aatree created from [hello; bye]"

gdziadkiewicz · 2022-12-18T12:43:59Z

tests/FSharpx.Collections.Experimental.Tests/AaTreeTest.fs

+            let array = [|1; 2; 3; 4; 5|]
+            let tree = AaTree.ofArray array
+            for i in array do
+                Expect.isTrue <| AaTree.exists i tree <| ""


Expect.containsAll?

gdziadkiewicz · 2022-12-18T12:46:57Z

tests/FSharpx.Collections.Experimental.Tests/AaTreeTest.fs

+            let inputList = [0;1;2;3]
+            let tree = AaTree.ofList inputList
+            let outputList = AaTree.toList tree
+            Expect.equal outputList inputList ""


Is it guaranteed that AaTree.toList output will be sorted?

gdziadkiewicz · 2022-12-18T19:58:16Z

src/FSharpx.Collections.Experimental/AaTree.fs

+open FSharpx.Collections
+open System.Collections.Generic
+
+(* Implementation guided by following paper: https://arxiv.org/pdf/1412.4882.pdf *)


I will be able to familiarize myself with it on Tuesday.

hummy123 · 2022-12-18T21:02:08Z

I think all of these are good suggestions and appreciate your rigorous standards. I'll have them implemented in the next few days hopefully.

gdziadkiewicz · 2022-12-19T09:55:49Z

src/FSharpx.Collections.Experimental/AaTree.fs

+
+    interface IEnumerable<'T> with
+        member x.GetEnumerator() =
+            (x.ToList() :> _ seq).GetEnumerator()


AATree.toSeq?

gdziadkiewicz · 2022-12-19T10:13:57Z

src/FSharpx.Collections.Experimental/AaTree.fs

+    let toList (tree: AaTree<'T>) =
+        foldBack (fun a e -> e::a) [] tree
+
+    let toSeq (tree: AaTree<'T>) =


Could we provide a lazy implementation? IMO it is unexpected and surprising for the user that just getting the seq, without even using it is O(n). Maybe something along the lines of:

let rec toSeq (tree: AaTree<'T>) = seq{ match tree with | E -> () | T(_, l, v, r) -> yield! toSeq l yield v yield! toSeq r }

?

gdziadkiewicz · 2022-12-19T20:35:53Z

tests/FSharpx.Collections.Experimental.Tests/AaTreeTest.fs

+open FSharpx.Collections
+open FSharpx.Collections.Experimental
+open Expecto
+open Expecto.Flip


I believe that you are not using the flip style and that the code will compile again if you remove it.

hummy123 · 2022-12-19T23:36:29Z

Hi @gdziadkiewicz and thanks again for the feedback. (I personally think it's easier to reply "here" rather than you jumping up/down the page navigating to different comments, but let me know if you prefer the other way of individual threads.)

I divided the tests to have one assertion each test and gave more descriptive test names as asked (good idea).
I used non-empty assertion messages in my new commit, but to be honest I think the messages I wrote are a bit tautologous with the test's name. (I didn't find much clear guidance on what to write from the other tests here like IntMap.fs but happy to revise them if asked.)
I tried to rewrite some/most of the compact tests to use AAA as recommended (good idea again).
You suggested using Expect.containsAll for some of the "Conversion from tests" (AaTree.ofList/Array/Seq), which I understand over the for loop that iterates over the input list and asserts AaTree.exists for each of them.

I think your way is definitely cleaner, but if we wanted to use Expect.containsAll we would need to use AaTree.toList/Array/Seq to build a list from the tree first.
I'm not sure if the mutual recursion with conversion methods (testing ofList with toList) is something I should worry about, but happy to make use of it in the test if asked (we have conversion toList/Array/Seq methods that build a list with of and check the output with to, so I think in that case we would be collapsing the conversion to/from tests since their logic would be exactly the same.

It's guaranteed that AaTree's conversion to methods will be sorted, as it does an in-order traversal of the AA Tree.
Good point. Replaced with toSeq as suggested, but the :> cast is still there (get a type inference error which I'm not sure how to solve in an OOP-style definition).
I haven't really worked with Sequences in F# (kind of new to the language/FP in general), so I don't feel confident writing a lazy toSeq implementation but I think it's a good idea. Happy to replace the current impplementation with your code (although I can't write test for a lazy function because of my inexperience with them).
Removed the open for Expecto.Flip as suggested and have the tests running successfully.

…/isabelle.in.tum.de/library/HOL/HOL-Data_Structures/AA_Set.html

hummy123 · 2023-01-02T06:30:05Z

I reviewed my code and compared it with the paper (and also with the Isabelle HOL proof linked below which corrects the paper in a couple of places) and made some changes.

Summary of my mistakes (now fixed):

My code only called the adjust function when deleting the left child when it should call the function in all cases (less than, greater than, equal).

Summary of changes from the proof (corrects errors in the paper, and corrections now implemented in code):

The function dellrg which the proof calls split_max recursed the left subtree in the original but is meant to recurse the right sutree.
Fixed calculation for the nlvl helper function.

Isabelle HOL proof for reference: https://isabelle.in.tum.de/library/HOL/HOL-Data_Structures/AA_Set.html .

gdziadkiewicz · 2023-01-04T16:15:17Z

@hummy123 the CI build is failing due to problems with code formatting. Could you resolve that?

gdziadkiewicz · 2023-01-04T16:21:44Z

src/FSharpx.Collections.Experimental/AaTree.fs

            then split <| (skew <| T(h, l, v, insert item r))
            else node

+    (* nlvl function fixed according to Isabelle HOL prrof below: *)


prrof -> proof

hummy123 · 2023-01-04T19:45:51Z

@gdziadkiewicz Got it working thanks to your help (haven't used FAKE/GitHub actions much before).

Here's the action running successfully on my own fork: https://github.com/hummy123/FSharpx.Collections/actions/runs/3841007372/jobs/6540731078 .

(Also updated FAKE as it was giving me errors.)

gdziadkiewicz · 2023-01-25T23:10:33Z

@hummy123 I'm sorry that I'm not able to work on it quicker. Would you mind if I add some property tests to your code and change the toSeq implementation to the one I proposed?

hummy123 · 2023-01-25T23:19:40Z

@gdziadkiewicz No worries - appreciate your time. Anything that makes the code better and tests more resilient sounds good to me, so go ahead.

Add AA Tree and tests

434122d

gdziadkiewicz self-assigned this Dec 13, 2022

gdziadkiewicz reviewed Dec 13, 2022

View reviewed changes

edit project file to include AaTree and tests, add labels for AaTree …

7990983

…constructor

gdziadkiewicz reviewed Dec 18, 2022

View reviewed changes

gdziadkiewicz reviewed Dec 19, 2022

View reviewed changes

hummy123 added 3 commits December 19, 2022 22:27

one Expect assertion for each test in AaTreeTest.fs

e7e1732

AAA structure for a couple of more tests

50d31b6

AaTree toSeq

9053d20

hummy123 added 2 commits January 2, 2023 05:50

fix incorrect functions in paper by Isabelle HOL proof below: https:/…

8ccd82f

…/isabelle.in.tum.de/library/HOL/HOL-Data_Structures/AA_Set.html

use isEmpty in delete case as in HOL proof

dda2bd0

Merge branch 'fsprojects:master' into master

cb72994

gdziadkiewicz reviewed Jan 4, 2023

View reviewed changes

hummy123 and others added 2 commits January 4, 2023 19:10

Merge branch 'fsprojects:master' into master

56bac48

fix formatting using FAKE and fix typo in comment

8d1a93c

fix my own implementation error in AaTree.splitMax (private function

171dd41

Conversation

hummy123 commented Dec 12, 2022

Uh oh!

gdziadkiewicz commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hummy123 commented Dec 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hummy123 commented Dec 19, 2022

Uh oh!

hummy123 commented Jan 2, 2023

Uh oh!

gdziadkiewicz commented Jan 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hummy123 commented Jan 4, 2023

Uh oh!

gdziadkiewicz commented Jan 25, 2023

Uh oh!

hummy123 commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants