-
Notifications
You must be signed in to change notification settings - Fork 115
Add a planarity notebook #146
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
base: main
Are you sure you want to change the base?
Conversation
It looks like this PR has a few extra files from isomorphism. These are probably additions you were making to update the Pr to match the main branch. I think it is probably best to "merge" or "rebase" the main branch changes into your branch rather than add them manually through commits and patches to the PR. To revert the recent commits that caused the problem you can Then to rebase/merge with the main branch
There may be places on the web that describe merge/rebase better than I have here. If you have troubles post here and we'll try to figure it out too. |
Yes, I followed the same procedure by resetting all the previous changes. After adding the planarity section, I merged it with the main branch to retrieve the remaining files. Now, I am at this stage. Please take a moment to review it. |
|
||
G: NetworkX graph | ||
counterexample : bool | ||
A Kuratowski subgraph (to proof non planarity) is only returned if set to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce the term "Kuratowski subgraph". Each time a technical term is introduced say what it is. In this case, perhaps you can say this when you introduce the theorem above.
typo?
A Kuratowski subgraph (to proof non planarity) is only returned if set to true. | |
A Kuratowski subgraph (to prove non planarity) is only returned if set to true. |
## Planarity Algorithm in NetworkX | ||
|
||
## Introduction | ||
A graph is said to be **planar** if it can be drawn on a plane without any of its edges crossing. The `check_planarity` function in NetworkX helps determine whether a given graph is planar and provides either an embedding or a counterexample. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explain what an embedding is. And what we mean by a counterexample.
### Mathematical Background | ||
A graph is **planar** if and only if it does not contain a subgraph homeomorphic to **K₅ (complete graph on 5 vertices)** or **K₃,₃ (complete bipartite graph with partition sizes 3 and 3)**. This is known as **Kuratowski’s theorem**. | ||
|
||
The planarity check in NetworkX is based on the **Left-Right Planarity Test**, an efficient combinatorial method to determine planarity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe briefly in a few sentences what the left-right planarity test does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure Sir, I will work on all the suggestions and make my Jupyter notebook more understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guide includes a lot of code, and some text. But the connection between the code and the text isn’t always clear, and the code is fairly hard to read. Can you run the NetworkX code but explain what it is doing in words — rather than making the reader read the code?
It would also be good to include more about why one would want todo this and why it is an important idea/question/problem.
```python | ||
#Example 1 | ||
# Load GraphML file | ||
G = nx.read_graphml("./data/planar_graph.graphml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create the graph in code for the small graphs in this notebook. That is more readable because they can see the nodes and edges without all the code and distraction of loading the file.
G = nx.Graph([(0, 1), (1, 2), (2, 3), (0, 2)])
|
||
|
||
```python | ||
from functools import singledispatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line of code is not needed/used in the notebook. Try to remove any spurious cells and code. :)
|
||
|
||
```python | ||
def is_planar(G): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these functions part of NetworkX? If so, you don’t need to define them here. You can call them from e.g. nx.is_planar
|
||
## Notes | ||
|
||
**Embedding** - A (combinatorial) embedding consists of cyclic orderings of the incident edges at each vertex. Given such an embedding there are multiple approaches discussed in literature to drawing the graph (subject to various constraints, e.g. integer coordinates), see e.g. [2]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an embedding is an assignment of coordinates in the plane to each node. The cyclic ordering of edges can be built as a way to find an embedding. Does combinatorial embedding mean the cyclic ordering even though it doesn’t given the coordinates? It is possible I am missing that key point. :)
|
||
|
||
|
||
A counterexample is only generated if the corresponding parameter is set, because the complexity of the counterexample generation is higher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if the complexity was the same, it would take significant time so the reason for making it optional is really about it taking time to do, the complexity isn’t as important as the extra time it takes. (which is related to the complexity, but those “constants” in the complexity description mean we can’t base time on complexity directly.
|
||
|
||
```python | ||
class PlanarEmbedding(nx.DiGraph): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including this code makes the notebook hard to read. It is code in NetworkX, so you can call it from that. And it would be good to explain it rather than making the reader figure out how the code works.
I recently submitted a pull request for the Planarity Notebook in the NetworkX repository. I noticed that there are some minor errors in my submission, and I sincerely appreciate your time in reviewing my work.
I am eager to contribute to this project and improve my submission to align with the repository's standards. Please let me know the necessary changes or improvements you'd like me to make, and I'll be happy to update the PR accordingly.
Looking forward to your feedback. Thank you for your guidance and for this opportunity—I’m truly excited to contribute and learn.
Fixes #122