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

Allow finding hamiltonian circuits #494

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

andybalaam
Copy link

This is an implementation of Frank Rubin's 1973 algorithm to compute Hamiltonian circuits in a directed graph.

It's O(n!) but it goes to great effort to reduce the speed at which execution time increases with n.

@ABorgna
Copy link
Member

ABorgna commented Jun 2, 2022

Hi.
The CI error is fixed in HEAD. Could you rebase your changes on the latest version ?

@andybalaam
Copy link
Author

@ABorgna rebased and pushed, thanks.

@andybalaam
Copy link
Author

@ABorgna hmm. The CI failed with the trait std::iter::IntoIterator is not implemented for &[({integer}, {integer}); 73]. Any idea why this works on my machine? Maybe I'm using a too-recent Rust version?

@ABorgna
Copy link
Member

ABorgna commented Jun 7, 2022

Yes, it breaks in rust 1.41 (the MSRV). Replacing &[...] with [...].iter() should do the trick.
You can also test it by changing the toolchain version locally: rustup override set 1.41 (and rustup override unset to go back).

Thanks for the implementation. I warn you that it will take some time to review :)

@andybalaam
Copy link
Author

I warn you that it will take some time to review :)

Of course, I absolutely expect that, and will help any way I can.

@andybalaam andybalaam marked this pull request as draft June 16, 2022 02:25
@andybalaam
Copy link
Author

I found a bug and am working on a fix, so marking this as draft for now.

Previously, it would skip too far ahead if we were processing the last
child of the node we wanted to skip.
Previously it would reject circuits that were actually the correct
Hamiltonian circuit!
@andybalaam
Copy link
Author

andybalaam commented Jun 16, 2022

To build with rust 1.41.0 I needed:

rustup override set 1.41
cargo update -p indexmap --precise 1.7.0
cargo test

Then later to revert back to normal rust:

rustup override unset

(And maybe something to undo the --precise from above too?)

For my own info, I discovered this by reading https://github.com/petgraph/petgraph/blob/master/.github/workflows/ci.yml

@andybalaam andybalaam marked this pull request as ready for review June 16, 2022 21:20
@andybalaam
Copy link
Author

OK, 2 bugs fixed, and compatibility with Rust 1.41 done!

The bugs have reduced my confidence in the tests though, so I'd welcome any suggestions on how to be more sure we are well-covered.

Both bugs made us not emit circuits that did exist, which is the harder case to test for. One possibility is to compare the list of circuits generated in the simplest possible way (without any of the R1...F6 conditions) against the list generated using the conditions. This would only be possible for small graphs, but might give us more confidence that the conditions are not excluding valid circuits.

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.

None yet

2 participants