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

Depth function #22

Merged
merged 6 commits into from
Oct 5, 2023
Merged

Depth function #22

merged 6 commits into from
Oct 5, 2023

Conversation

harshey1103
Copy link
Contributor

Initial implementation of depth function.
The function returns 0 for an empty tree and 1 for a tree whose root is a leaf.

Copy link

@AryanGodara AryanGodara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it'll be a good idea to write a test for this new function. What do you say @harshey1103 :)

src/rtree.ml Show resolved Hide resolved
Copy link
Contributor

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks @harshey1103 ! This looks good to me, I agree that a test or two would be great. The OMT loading algorithm that we support should produce a balanced tree so that can be used for the tests.

Also running dune build @fmt --auto will format the code :))

@patricoferris
Copy link
Contributor

Just as an additional thing, your depth' function isn't tail-recursive. I'm totally fine with this but it might be a nice exercise to see if you can make it tail-recursive. Some extra info on that: https://www.cs.cornell.edu/courses/cs3110/2014fa/recitations/5/rec05.html

@harshey1103
Copy link
Contributor Author

Hi @patricoferris, I looked into the test directory but I am not able to figure out how to write a test for the depth function. There aren't any tests for the Rtree module. :'(

@patricoferris
Copy link
Contributor

Yes the tests are a little disorganised, apologies! There are tests of the Rtree module though. For example

ocaml-rtree/test/basic.ml

Lines 88 to 120 in ee12c37

let test_lines () =
let module R =
Rtree.Make
(Rtree.Rectangle)
(struct
type t = line
let t = lint_t
type envelope = Rtree.Rectangle.t
let envelope { p1 = x1, y1; p2 = x2, y2 } =
let x0 = Float.min x1 x2 in
let x1 = Float.max x1 x2 in
let y0 = Float.min y1 y2 in
let y1 = Float.max y1 y2 in
Rtree.Rectangle.v ~x0 ~y0 ~x1 ~y1
end)
in
let l1 = { p1 = (1., 2.); p2 = (2., 3.) } in
let index = R.insert (R.empty 8) l1 in
let l1' = R.find index (Rtree.Rectangle.v ~x0:0. ~y0:0. ~x1:3. ~y1:3.) in
assert (List.length l1' = 1);
assert (l1 = List.hd l1');
let l2 = { p1 = (3., 4.); p2 = (5., 5.) } in
let index = R.insert index l2 in
let l1' = R.find index (Rtree.Rectangle.v ~x0:0. ~y0:0. ~x1:3. ~y1:3.) in
assert (List.length l1' = 1);
assert (l1 = List.hd l1');
let l1' = R.find index (Rtree.Rectangle.v ~x0:0. ~y0:0. ~x1:5. ~y1:5.) in
assert (List.length l1' = 2);
let vs = R.values index in
assert (List.length vs = 2)

Here we test the OMT loader for an instantiation of the Rtree module using rectangles and lines. This should help write a test to check the depth function :))

@harshey1103
Copy link
Contributor Author

harshey1103 commented Oct 5, 2023

I tried to make the function tail recursive and have written the tests can you please have a look @patricoferris @AryanGodara .

@harshey1103 harshey1103 marked this pull request as ready for review October 5, 2023 09:13
@patricoferris
Copy link
Contributor

The changes are looking good to me. Our CI system seems a bit broken, so I've opened an issue for them: ocurrent/solver-service#74. I ran this locally and I'm happing with the changes, thank you @harshey1103 :))

@patricoferris patricoferris merged commit 7e00275 into geocaml:main Oct 5, 2023
1 check failed
@patricoferris
Copy link
Contributor

One thing I would add is that the depth function is not quite tail-recursive here (the function call is not in the tail position) but that's okay, I think something else might break before we get depths that cause us trouble... I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants