Docs on graph analysis #314
Conversation
|
Alright, this is a good first stub. Of course, documentation can always get better, but I feel like at least now we have a compass for users to start using the library! This does not really need three reviews of course, it's more like an offer to open for comments. We could happily merge as is. |
|
Response to the review request: I can comment on the content in general terms, but I am not sufficiently familiar with the Python interface to comment on anything specific to that. Take my comments as suggestions. |
doc/source/analysis.rst
Outdated
|
|
||
| To remove nodes, use :meth:`Graph.delete_vertices`: | ||
|
|
||
| >>> g.delete_vertices(None) # remove all vertices |
There was a problem hiding this comment.
Naïve question from someone who doesn't use the Python interface much: How would I figure out from the docs that g.delete_vertices(None) deletes all vertices?
There was a problem hiding this comment.
Hmmm, I did not know that g.delete_vertices(None) deletes all the vertices -- it was never intended. It is an unintended consequence of the fact that the function takes something-that-can-be-converted-into-an-igraph_vs_t and that igraphmodule_PyObject_to_vs_t() converts None to "all vertices".
Maybe we should update delete_vertices() to throw an error for None as I don't like this syntax. I'd rather have g.clear() (but we don't have it now, and it would also need to delete all the graph attributes as well).
There was a problem hiding this comment.
Haha yeah this one was surprising for me too. I'll remove this from here and start a separate PR implementing g.clear.
I don't think the usage for deleting all vertices but leaving the graph attributes will be heavy anyway.
There was a problem hiding this comment.
igraphmodule_PyObject_to_vs_t()convertsNoneto "all vertices".
Yeah, that reads really funny in English: "Which vertices?" "None" Then it takes all of them instead of none of them.
There was a problem hiding this comment.
Okay, the reason is as follows. There are many methods like degree() that work on the whole graph by default but that may take a vertex ID or a list of vertex IDs to operate on a subset of vertices instead. For these methods, the vs argument needs a default value that we then treat as "all vertices". I took a commonly used shortcut and simply used None (which is basically null or nil in Python), that's why None is converted to "all vertices" by igraphmodule_PyObject_to_vs_t(). An alternative (and probably a cleaner way) would have been to create a singleton object named ALL and then make this the default value instead of None. We can add this as a low-priority issue and we can deal with it in the CZI grant if we get to the point where we need to clean up the Python API before 1.0. There would also have to be a deprecation phase where we still allow None but show a warning to the user and ask him to use ALL instead.
|
I added a few comments. I hope some are helpful, but it's up to you to make changes or leave things as they are. |
|
Added a few comments myself as well. |
|
Awesome. Any more comments @szhorvat? Shall we merge this? |
|
I don't have any more comments. |
As done previously for graph generation, here a PR adding docs on graph analysis.
This one is a little less obvious because "graph analysis" can mean so many things. Vice versa, it helps us identify obvious holes in the Python interface such as DFS.
Please let me know what topics you would like me to write on.
Cheers,
Fabio