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

Fix tests of kd_tree #18

Closed
wucke13 opened this issue Nov 25, 2021 · 4 comments
Closed

Fix tests of kd_tree #18

wucke13 opened this issue Nov 25, 2021 · 4 comments
Assignees

Comments

@wucke13
Copy link
Member

wucke13 commented Nov 25, 2021

This test is broken. When playing with the numbers, I was able to create a stack overflow. This should not be possible. Furthermore, I would advice to check the numbers - while it makes sense to have different test sizes for debug and release builds, I'm not sure that this should be a test at all. It is rather three things:

  • A fuzzy test. For this, why don't we implement a stub for cargo fuzz instead? Then also one can run the test for arbitrary times without having to alter the test code.
  • A performance regression test. For this, it might make sense to integrate either the cargo bench feature or criterion.rs
  • A unit test checking the correctness of the tree generation (?) and lookup. In this case, this should be separated into multiple tests, testing each of the two properties isolated.

I'm not so comfortable having code this complicated without rigid validation. Hence I would also propose that we introduce tests which uses one of the other kd tree implementations of the rust universe as oracle to check our implementation.

@wucke13 wucke13 assigned wucke13 and sevenautumns and unassigned sevenautumns and wucke13 Nov 25, 2021
@sevenautumns
Copy link
Collaborator

I'm not so comfortable having code this complicated without rigid validation. Hence I would also propose that we introduce tests which uses one of the other kd tree implementations of the rust universe

The KD Tree implementation is already tested against a primitive linear search.
Imo there is no need for testing it against more implementations

@wucke13
Copy link
Member Author

wucke13 commented Nov 26, 2021

I think the order of correctness guaranteed by having all queries beeing correct, or knowing that the tree is correctly implemented is not equal. The former could have bugs in the creation of the tree which are solved by other bugs in the query. The latter checks both individually. However, I agree, this is low priority ATM. I think we can look into this again after #22 is solved.

@sevenautumns
Copy link
Collaborator

The Tree we implemented here results in a non-standard representation of the tree as a specially balanced heap. There will be no present kd tree implementation resulting in the same tree

@wucke13
Copy link
Member Author

wucke13 commented Nov 26, 2021

That's a good reason not to check against other implementations. However, can we have a test case which (under heavy use of heap memory) traverses the tree to check its validity as unit test?

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

No branches or pull requests

2 participants