Remove fns

[FayCarsons]

Implemented removal by equality and by envelope in a way I'm pretty happy about.

Testing is needed (which, I'm not super sure how to go about that) and I think the aux functions for both can be combined into a single removal by predicate fn? But there's also potential branching optimizations in the case of the removal by envelope operation that may clash with that? Will investigate!

[patricoferris]

This is looking great, thanks @FayCarsons !!

In terms of testing, I think we can add some unit tests to basic.ml. Maybe something like:

let test_remove () =
  let module R = R1 in
  let lines =
    [
      { p1 = (0., 0.); p2 = (1., 1.) };
      { p1 = (1., 1.); p2 = (2., 2.) };
      { p1 = (2., 2.); p2 = (3., 3.) };
      { p1 = (3., 3.); p2 = (4., 4.) };
    ]
  in
  let idx = R.load ~max_node_load:2 lines in
  let t' = R.remove_eq idx (List.hd lines) in
  match t' with
    | None -> failwith "Unexpected none returned from remove"
    | Some (vs, t') ->
    assert (R.size t' = 3);
    assert (List.hd vs = List.hd lines)

There might nicer/more rigorous tests too.

[patricoferris]

We'll probably want to expose this in the Rtree interface (module type S) as something like:

val remove_env : t -> Envelope.t -> (Value.t list * t) option

In which case the implementation will have to unwrap the tree and rewrap it as a t, something like:

Suggested change

	match remove_env' env t with [], _ -> None | elts, t' -> Some (elts, t')
	match remove_env' env t.tree with
	| [], _ -> None
	| elts, t' -> Some (elts, { t with tree = t' })

[patricoferris]

Similarly for remove_eq

[patricoferris]

Suggested change

	let remove_eq t ty e =
	let remove_eq t e =

Note that we don't have to explicitly pass the runtime type into this function. Instead, it has already been made available as an argument to the functor we are currently inside, so we can just pass V.t directly to Repr.equal.

[patricoferris]

Sorry for the delay!

[FayCarsons]

Thanks for the feedback! Applied suggestions and added unit test, will try to add more later when I can take a closer look at the test suite.

[FayCarsons]

Added more unit tests. Testing removal on an empty tree, removal from a tree w/ one elt, removing an elt that doesn't exist, removing a lot of elts. I wanted to add a test for removing from a degenerate tree but wasn't sure how to construct a degenerate tree. Can explore later. Fixed doc comments.

[FayCarsons]

Added another unit test that inserts 100 random elts, half within an envelope and half out, and then removes the half within the envelope.

I'm not super experienced with unit testing, would love any suggestions for new unit tests if this is not comprehensive enough!

[FayCarsons]

Hi @patricoferris ! Was just reminded of this PR and was wondering if there's anything I can do to get it merged?