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 Left-Right Planarity test. #475

Merged
merged 12 commits into from
Sep 29, 2022
Merged

Conversation

georgios-ts
Copy link
Collaborator

This commit implements a new function is_planar that
checks if an undirected graph can be drawn in a plane without any edge
intersections. The planarity check algorithm is based on the
Left-Right Planarity Test.

Reference: http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.217.9208

This commit implements a new function `is_planar` that
checks if an undirected graph can be drawn in a plane without any edge
intersections. The planarity check algorithm is based on the
Left-Right Planarity Test.

Reference: http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.217.9208
@georgios-ts
Copy link
Collaborator Author

Marking this on hold since it depends on #456.

@coveralls
Copy link

coveralls commented Oct 25, 2021

Pull Request Test Coverage Report for Build 2064399819

  • 425 of 433 (98.15%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 98.401%

Changes Missing Coverage Covered Lines Changed/Added Lines %
retworkx-core/src/planar/lr_planar.rs 420 428 98.13%
Totals Coverage Status
Change from base Build 2041483593: -0.01%
Covered Lines: 11384
Relevant Lines: 11569

💛 - Coveralls

@IvanIsCoding
Copy link
Collaborator

After #497, you can use the new generator to make some test cases. Table 2 of https://www.sciencedirect.com/science/article/pii/S0166218X08000371 has the crossing numbers of the Petersen graphs, zeros in the table are planar and the rest aren't

@IvanIsCoding IvanIsCoding added this to the 0.12.0 milestone Jan 11, 2022
@mtreinish
Copy link
Member

@georgios-ts is there any update on this? It would be good to get this rebased as @enavarro51 is starting to look at adding a planar_layout() method and having some functionality around lr planarity in retworkx-core would be a good starting off point for that

@georgios-ts
Copy link
Collaborator Author

@mtreinish This should be ready for review now.

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

@georgios-ts. It was suggested by @mtreinish that I review this PR more as a process of learning. To start with some high-level questions

  • is_planar is in retworkx-core. Is there a reason it's there instead of retworkx? It seems this would be a function accessed externally the same as planar_layout might be.
  • The networkx approach creates the embedding at the same time the planarity test is being done. Is there a reason you only did the planarity test? As in, do you think the creation of the embedding should be separate from the planarity test, or incorporated within it?
  • My current plan is to create a PlanarEmbedding struct and associated methods mirroring what is done in networkx and then when your PR is merged, to add the hooks to create the embedding within lr_planar.rs code. This would be followed by writing the combinatorial_embedding_to_pos function. Does this seem like a reasonable approach?
  • Thanks.

retworkx-core/src/planar/lr_planar.rs Outdated Show resolved Hide resolved
retworkx-core/src/planar/lr_planar.rs Outdated Show resolved Hide resolved
retworkx-core/src/planar/lr_planar.rs Outdated Show resolved Hide resolved
tests/graph/test_planar.py Outdated Show resolved Hide resolved
@georgios-ts
Copy link
Collaborator Author

@enavarro51 Thanks for taking the time to look into this.

  • It's in retworkx-core so we can expose it in our public rust API but there is also a python interface in retworkx that uses internally the implementation written in retworkx-core.
  • As you said, to create the embedding we need a PlanarEmbedding structure. This was additional work and I didn't want to overwhelm this PR since it was already quite long. But yeah, I do believe that the creation of the embedding should be separate from the public is_planar test. IMO, we should have two public functions: a test that returns true/false for planar/non-planar graphs and a method that creates a planar layout. All other function/structures (e.g PlanarEmbedding) should be private.
  • It sounds like a good plan. After implementing PlanarEmbedding you can leverage the lr_visit_ordered_dfs_tree function to implement Algorithm 6 in the linked paper (the NetworkX equivalent https://github.com/networkx/networkx/blob/main/networkx/algorithms/planarity.py#L650).

If you have any other questions feel free to reach out.

Copy link
Collaborator

@itoko itoko left a comment

Choose a reason for hiding this comment

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

Thanks, @georgios-ts (and good QAs with @enavarro51, the previous comments helped my review a lot.) This PR looks good to me. I mainly reviewed it from an algorithmic point of view especially comparing with the networkx implementation. It implements a planarity check algorithm using the visitor pattern in two depth-first searches nicely. I think that is making the code quite readable and also useful for another PRs such as #645. I've left a few minor comments inline but I think none of them would prevent from merging this after resolving conflicts.

use retworkx_core::petgraph::graph::UnGraph;
use retworkx_core::planar::is_planar;

#[test]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those tests look well-isolated to the module so we could put them in #[cfg(test)] block in /src/planar/lr_planar.rs as suggested in #587 (comment)

retworkx-core/src/planar/lr_planar.rs Outdated Show resolved Hide resolved
tests/graph/test_planar.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 22, 2022

Pull Request Test Coverage Report for Build 3148280248

  • 423 of 431 (98.14%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 97.095%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/planar/lr_planar.rs 418 426 98.12%
Totals Coverage Status
Change from base Build 3129997168: -0.01%
Covered Lines: 13135
Relevant Lines: 13528

💛 - 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.

LGTM, I think having readable code for a long algorithm implementation like this one is a huge plus.

I am also confident on the implementation because of the tests. I think we cover a good amount of them, and further in #645 we have planar layout tests building on top of this so I believe this is good to merge

@mtreinish mtreinish modified the milestones: 0.13.0, 0.12.0 Sep 29, 2022
@mtreinish mtreinish added the automerge Queue a approved PR for merging label Sep 29, 2022
@mergify mergify bot merged commit 5b50f3f into Qiskit:main Sep 29, 2022
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.

6 participants