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

Add two_color and is_bipartite #1002

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Add two_color and is_bipartite #1002

merged 4 commits into from
Oct 16, 2023

Conversation

mtreinish
Copy link
Member

This commit adds new functions to rustworkx, two_color() and
is_bipartite(), which are used to compute a two coloring for a graph and
then determine if a givn graph is bipartite. The two_color() function is
added to rustworkx-core as the python is_bipartite() function just wraps
it and converts the output to a bool if a two coloring is possible or
not.

This commit is based on top of #998 and will need to be rebased after
that merges.

You can see a diff of just the contents of this PR here:

https://github.com/Qiskit/rustworkx/compare/mtreinish:retworkx:add-isolates...mtreinish:retworkx:add-two-color?expand=1

@coveralls
Copy link

coveralls commented Oct 11, 2023

Pull Request Test Coverage Report for Build 6502172051

  • 65 of 65 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 96.52%

Totals Coverage Status
Change from base Build 6490295783: 0.02%
Covered Lines: 15532
Relevant Lines: 16092

💛 - Coveralls

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

The algorithm LGTM but we could add some test cases using generators to have a more robust test suite.

I left comments for Generalized Petersen Graphs, Grid Graphs, and Odd Cycle Graphs

/// expected_colors.insert(NodeIndex::new(4), 1);
/// assert_eq!(coloring, expected_colors)
/// ```
pub fn two_color<G>(graph: G) -> Option<DictMap<G::NodeId, u8>>
Copy link
Contributor

Choose a reason for hiding this comment

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

A general question about Rust: is there a performance penalty from using DictMap vs a vector (indexed by NodeIndex or something), i.e. is lookup / insertion into DictMap of order O(1) or of O(log n)?

Copy link
Member Author

Choose a reason for hiding this comment

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

DictMap is basically just a IndexMap alias with a custom default hash algorithm. So lookup/insertion is O(1) but unlike a HashMap it retains insertion order. I went for using a dict map here because there is no guarantee that the node indices are contiguous or a sequence. The simplest example of that is if the input graph is a StableGraph with nodes removed in the middle. In general I'd expect a Vec would be faster here, but since we need this function to be general to handle all sorts of different types of graphs this seemed like the best option for the return type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s slower than a vector but still pretty fast (O(1)). It keeps the insertion order but that is what users expect from a Python dictionary-like structure

This commit adds new functions to rustworkx, two_color() and
is_bipartite(), which are used to compute a two coloring for a graph and
then determine if a givn graph is bipartite. The two_color() function is
added to rustworkx-core as the python is_bipartite() function just wraps
it and converts the output to a bool if a two coloring is possible or
not.

This commit is based on top of Qiskit#998 and will need to be rebased after
that merges.
/// expected_colors.insert(NodeIndex::new(4), 1);
/// assert_eq!(coloring, expected_colors)
/// ```
pub fn two_color<G>(graph: G) -> Option<DictMap<G::NodeId, u8>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s slower than a vector but still pretty fast (O(1)). It keeps the insertion order but that is what users expect from a Python dictionary-like structure

@mtreinish mtreinish removed the on hold label Oct 13, 2023
@mtreinish mtreinish added the automerge Queue a approved PR for merging label Oct 16, 2023
@mergify mergify bot merged commit de3eb0c into Qiskit:main Oct 16, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants