adding extendability problem (2nd try)

[gsemer]

No description provided.

[dschult]

Can you add your new function to the doc/reference/algorithms/bipartite.rst file? Use the same form as for the other modules. Your's has a single function in it so it will be short. Be careful of the spacing to match that of the other modules being documented in there.

That way the autodoc system will find your function and slip the docstring into the main documentation.

Also, please add your module and function to the __init__.py file in the bipartite directory. Use a similar syntax as for the other modules. You should be able to say

import networkx as nx
G=nx.path_graph(4)
nx.bipartite.extendability(G)

[jarrodmillman]

Igraph made a new release right before you updated your PR:

• https://pypi.org/project/python-igraph/

I haven't looked into it yet, but the error is not related to what you did. One of us will sort out the igraph issue and let you know what to do next.

[dschult]

This looks good... Can you check out the docs (starting at the bipartitie reference to find_extendability) These are available from the final test "Details" link. The final test is a "Build Artifact" -- that is, something created during the Circle CI test -- it is a build of the documentation webpages. Anyway, you can see what the docs will look like in the online documentation and change things if you want.

It looks like the docstring needs different underlines on the sections:

Parameters    
----------
    G : NetworkX Graph
----------

to similar but different:

Parameters    
----------

    G : NetworkX Graph

Similar changes to the other sections will help them format nicely too.

[gsemer]

I followed your instructions and made all necessary changes. Also, i changed the docs. It is not nice to see an example on the documentation webpage for bipartite module. It seems the tests did not fail this time and worked properly.

[gsemer]

I got an error again. Please give me instructions how to fix it when you are avalaible.

[dschult]

I'm not sure what exactly is causing tis error, but it is certainly not your code.
So, the way I would try to solve it is to merge the networkx main branch into your PR.
It is covered in the CONTRIBUTING.rst file at the base directory of the repository.
But in short:

# add the remote if you haven't before... 
git remote add upstream https://github.com/networkx/networkx.git
git remote -v  # check that it worked

git branch -v  # make sure you are on the branch you want to update
git pull upstream main

# there may be some conflicts when merging if someone has made changes on the main branch to files you are changing.
# edit those files to make sure you don't un-do whatever someone else did that conflicted with your work.
git status

git add ...
git commit -a
git push origin <branchname>  # or just `git push` if you aren't doing other stuff in other branches you don't want pushed

[gsemer]

So i guess that can not contribute to that error.
Will i have to wait for a comment in case there is something new ?

[rossbar]

So i guess that can not contribute to that error.

Correct, the error was unrelated to your changes.

Will i have to wait for a comment in case there is something new ?

The problem has been fixed on main, so you can fix in your PR by merging main into your extendability branch. As Dan mentions above, the easiest way to do this is git pull upstream main. After that, you can git push the result and the CI will run again.

[gsemer]

Thank you. It worked this time

[gsemer]

Unfortunately, an error occured again.

[rossbar]

I haven't had a chance to take a detailed look, but a couple things jumped out to me as potential improvements:

• I think the amount of comments in the code can be significantly reduced to improve readability. One of the nice things about Python is that the code itself is very readable, so adding explicit commenting is often unnecessary. Of course, there's a bit of subjectivity to this, but a good rule of thumb is "if I came back to this code in 6 months, would the additional comment help me understand what's happening".
• Re: the tests --- they seem to be quite specific; are they derived from somewhere specific, e.g. one of the papers referenced in the docstring? If so, it would be nice to reference them in test docstrings.
• Also re: the tests - I'm not 100% sure, but from a quick glance it seems that explicitly specifying the nodes is unnecessary (i.e. the add_nodes_from step could be skipped). It would only be necessary if you wanted to add nodes that were not connected, but AFAICT that's never the case. Also, it's a minor thing but you could save quite a few characters by using integer nodes instead of strings!

I'll take a closer look as soon as I have a chance.

[gsemer]

By mentioning that the amount of comments can significantly reduced, did you refer to the ones made in the code or the theory part ?
The tests are actually examples made by me and not by the papers. I am sure that they are correct. In case it will be helpful, i could mention one more theorem which helps anyone in constructing a k-extendable graph.
What do you suggest me to do in order to fix the last mentioned problem ( replace add_nodes_from ) ?

[rossbar]

By mentioning that the amount of comments can significantly reduced, did you refer to the ones made in the code or the theory part ?

Good question - I was referring to some of the inline comments in the code itself. See below for an example. I've left one example, but I think you could trim quite a few more of the inline comments along the same lines.

The tests are actually examples made by me and not by the papers. I am sure that they are correct. In case it will be helpful, i could mention one more theorem which helps anyone in constructing a k-extendable graph.

Got it, thanks for the info. An additional not on how the tests are formulated would be useful I think, but not necessary!

What do you suggest me to do in order to fix the last mentioned problem ( replace add_nodes_from ) ?

The add_edges_from method automatically adds any nodes from the edges themselves if the nodes are not already present in the graph. Therefore, you should be able to eliminate the nodelist and the call to add_nodes_from from the tests --- see example below. There are some potential exceptions to this, e.g. if the ordering of the nodes matters or if you are trying to add nodes that are not connected to any other edges, but AFAICT that's not the case.

[gsemer]

I guess that all the necessary changes have been done. Please let me know if you would like to make any additional change.
How long will it take approximately for you to merge the code ? I am asking because i would like to include this kind of implementation in my diploma thesis.

[gsemer]

I was wondering about whether the code is going to be added in networkx library.
I am asking because a month has passed since the last message and am curious to hear from you.

[rossbar]

I was wondering about whether the code is going to be added in networkx library.

Thanks for the ping and sorry for the delay; the release of NetworkX v2.6 has taken up a lot of time recently and we haven't had a chance yet to switch gears back to reviewing new contributions for later releases. I aim to give this PR another review as soon as I can.

[dschult]

Thanks for this!
I guess the main big questions in this review pertain to whether you might want to include other related functions. That might be beyond the scope of this PR though... Thanks for this function.

[dschult]

Can you add a sentence or two after the Lemma to explain why/how this lemma relates to this function? Are you explaining the algorithm you use -- there must be a reason to include this lemma at this point.

This lemma also leads to a question of whether you should provide a function that returns the residual graph.

[gsemer]

Actually extendability is defined as the maximum k for which the graph is k-extendable.
And this helps someone to determine whether a graph G is k'-extendable or not. You can just calculate the maximum value k as mentioned before and check whether k' is less than or equal to k. So I think that there can be exist an additional function is_k_extendable(G, k) that will return a boolean True or False(G must be always satisfies the conditions of the extendability funcion of course).
I am looking forward to hearing your response.

[rossbar]

I took the liberty of pushing up some formatting-related changes to the docstring. I had a few other questions/suggestions from another pass through the implementation.

One general note about the tests: I noticed that some of the graphs used can actually be constructed using networkx generators. IMO it's an improvement to specify the test graphs in this way when possible, as it makes the test much more readable! I left an explicit suggestion for the first, but would recommend repeating that pattern as far as possible for the other test cases (e.g. I noticed the second test is the Platonic cubical graph).

[dschult]

A good canonical article to link to for k-extendable graphs is:
M. D. Plummer. On n-extendible graphs. Discrete Mathematics, 31:201–210, 1980 https://doi.org/10.1016/0012-365X(80)90037-0

As for names, the original article uses n-extendability to be the trait of being n-extendable. So the n-extendability of G is True iff G is n-extendable. The quantity in this function is the maximal n such that G is n-extendable. I think a better name for the function would be:
maximal_extendability()

Thoughts? (These two items fulfill my missions stated above, so I have checked them in the comment.)

[rossbar]

This LGTM too, thanks @gsemer and @dschult !

I took the liberty of pushing up a few minor changes: some docstring formatting things in 659138a , and a few additional tests for exception-raising branches of the code in 4188a13 . Hopefully these aren't too controversial, but don't let them be a blocker either - feel free to reset and merge without them!

[dschult]

Nice changes! Thanks!
Looks ready for merging to me.