aldanor / chappie

Search algorithms in Rust
Apache License 2.0
1 stars 1 forks source link

Add randomized graph tests #7

Closed aldanor closed 9 years ago

aldanor commented 9 years ago

Based by #6 but includes exhaustive correctness check for all valid paths + some refactoring.

jpastuszek commented 9 years ago

Cool! This is exactly as I was hoping this to become. Thanks for taking it to the next level :) for loops are sometimes way more readable than zip() and friends. Also I like use of const for naming numbers - makes it more readable. I was trying to use [dev-dependencies] for rand but I could not get it working - we should not need to add dependency that is not used for normal build. I think the test function does sufficient testing and this graph actually tests Visited.

aldanor commented 9 years ago

Hmmm, I'll check it out later, but it should work in theory: http://doc.crates.io/manifest.html#the-[dev-dependencies.*]-sections

There's one thing left to be done here though (ideally) -- verifying that if None was returned, there is actually no path from start to goal.

aldanor commented 9 years ago

@jpastuszek I fixed dev dependency -- you should have put #[cfg(test)] before extern crate rand of course, otherwise it'd try to link it in for the normal build as well. We also run benchmarks on Travis now to make sure they compile and execute fine.

I've also added a state observer example which lets us inspect which states were visited and verify correctness of dfs returning None. In order to do this, though, I had to change dfs's signature to accept arguments by reference -- but it probably makes sense anyway? (because we don't really need to transfer ownership there)

There's probably a nicer way around this, like using the Borrow trait like HashMap does, but maybe this should be the next step.

aldanor commented 9 years ago

Will post it here because it's cool :) The graph that's used in tests to verify that everything's correct:

foo dot

jpastuszek commented 9 years ago

Looks cool. I was thinking about ToOwned trait if we want to work with both by-value and by-pointer. We could also take everything by reference and keep pointers in visited but they must outlive call to dfs() if we are to drop Clone requirement so states must be stored somewhere for duration of search.

aldanor commented 9 years ago

How'd you like the RefCell hack? :)

I think I'll merge this in for now (although I'm not 100% happy with the whole reference/pointer situation -- it's a bit of a mess, but as you said we need to figure it out to make it more convenient to call so that a value or a reference can be passed transparently).

jpastuszek commented 9 years ago

I think I know why you need it :D Looks bit messy atm but it is going in the right direction.

aldanor commented 9 years ago

Why RefCell is needed? To provide interior mutability to an otherwise immutable structure, I think that part is quite clean actually.

Re: references, yea, I think the first step is making everything (or most everything) a reference, and then trying to find a nice way to wrap it (Borrow? Cow? ToOwned?), not the other way around. Maybe some trait bounds are too strict as well...