-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Added tree recognition functionality #925
Conversation
""" | ||
|
||
if type(G) == nx.Graph: | ||
return nx.number_of_nodes(G) == 0 or (nx.is_connected(G) and ( nx.number_of_edges(G) == nx.number_of_nodes(G)-1 ) and G.number_of_selfloops()==0) |
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.
The last check, G.number_of_selfloops()==0
, is useless: a connected graph with self-loops necessarily has more than n - 1 edges.
On a different note, try to format the code so that line length is at most 79 characters.
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 are completely right. I have made a new commit to remove the useless check and improve the formatting.
I am wondering if creating a new package for this is not overkill. If only a few functions would end up in this tree/forrest recognition, it is probably better to just make it a module in the algorithms package. @Markusian how many more such functions were you thinking about? |
I think that you are probably right. My initial plan was to implement a tree and a forest recognition and then see if someone else has other ideas that could be implemented. |
For undirected graphs only. | ||
""" | ||
|
||
if type(G) == nx.Graph: |
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.
Usually tests using isinstance are preferable since subclasses of nx.Graph will pass as well:
if isinstance(G, nx.Graph):
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 better in this case would be to check if the graph is directed using G.is_directed().
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 was not sure of type either, but using isinstance was a problem for inheritance as DiGraph is a subclass of Graph.
I think that it is a very good idea to use is_directed
…f the input graph with the appropriate is_directed() function. With this change, graph of the class MultiGraph can be correctly recognised as tree too. The test suite has been modified accordingly
Slightly modified the is_tree() function.
Simplify logic using not_implemented_for() Recognize pointless concept
Markusian trees forests recognition
Markusian trees forests recognition
Added tree recognition functionality
I have written a simple function to recognise if a certain graph is a tree or not. If the following is true, the input graph is considered a tree:
In my implementation, an empty graph is considered a tree too.
This is my first attempt to collaborate on this project and also to an open source project in general, so let me know if I'm going in the right direction, if I need to modify something, what I'm doing wrong and so on. I have also tried to write a series of tests for the function.
If I'm going well I would like to add some other small functions, like a forest recognition. I know that these are really simple functions but I've encountered the need of them in a school project and I had to implement them myself, using a really stupid implementation at that time. So I think that this could be useful to someone else.
So, be kind with me and let me know what you think :)