Opened 7 years ago
Closed 7 years ago
#12806 closed defect (fixed)
upgrade of spkg networkx-1.2.p2 to 1.6
Reported by: | dkrenn | Owned by: | jason, ncohen, rlm |
---|---|---|---|
Priority: | major | Milestone: | sage-5.2 |
Component: | graph theory | Keywords: | spkg networkx graphs |
Cc: | kini, fbissey, brunellus, ncohen | Merged in: | sage-5.2.beta1 |
Authors: | Daniel Krenn, Javier López Peña | Reviewers: | Keshav Kini, Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
When upgrading the networkx package to 1.6, there arise a lot of doctest errors, see discussion on sage-devel. This ticket should contain everything that is necessary to do so.
To get the doctests passing, the following modifications have been made:
- Fixed the calls in
graph.py
to conform to the new API forbetweennness_centrality
. Also exposed the new options. - Failing doctest in
digraph.py
was shocasing a bug that isn't there anymore. Fixed and documented it, and exposed the previously offending implementation to sage - Two failures in
graph_generators.py
were due to changes in randomized methods, so just changed the tests to conform to the new output, the rest were due to a bad-parameter case that was used to raise an exception but now degenerates gracefully. Fixed the corresponding doctests to account for that. - Failures in
graph_generic.py
were due to a change in the output of clustering coefficients for weighted graphs. Added an optional parameter to use the new output, implemented the old behaviour as it was in NetworkX 1.2, and set the old behaviour to default with a deprecation warning.
To review this ticket:
Attachments (3)
Change History (30)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
- Cc kini added
comment:3 Changed 7 years ago by
- Cc fbissey added
comment:4 Changed 7 years ago by
- Cc brunellus added
comment:5 Changed 7 years ago by
The failing tests in graph.py are due to an API change in the betweenness_centrality function. They get fixed by replacing
return networkx.betweenness_centrality(self.networkx_graph(copy=False), normalized)
in line 3252 of SAGE_ROOT/devel/sage-main/sage/graphs/graph.py by
return networkx.betweenness_centrality(self.networkx_graph(copy=False), normalized = normalized)
I will take a look at the rest of the failing tests tomorrow and write a patch.
comment:6 Changed 7 years ago by
The failing test in digraph.py is an improvement. In older versions of NetworkX topological_sort_recursive
had a problem that has since been sorted out. The failing test was showcasing a bug that is not there anymore. This is also good news since we can also expose the recursive implementation of topological sorts in sage.
comment:7 Changed 7 years ago by
All but two of the failing tests in graph_generators.py
are due to extreme cases of balanced trees (degree 1, height 0), that didn't use to be allowed before and now degenerate gracefully. So this problem gets sorted out just by removing the offending doctests. As a nice side-effect, the offending tests took most of the testing time before. After removing them time for running the tests in graph_generators.py
drops for me from 357s to 18s.
comment:8 Changed 7 years ago by
The two remaining doctests in graph_generators.py
are due to changes in two randomized methods: a change in networkx.powerlaw_cluster_graph
from using random.random
to _random_subset
and a rewriting of the random method expected_degree_graph
. Since the functions called in the doctests have a randomized nature it is not unreasonable to expect the results to change after the randomized function is rewritten. Unless somebody complains I will just update the tests to the results with the new methods.
comment:9 Changed 7 years ago by
- Cc ncohen added
Changed 7 years ago by
comment:10 Changed 7 years ago by
Added a patch with the necessary changes for the doctests. All tests pass in my system. To review this patch you need to install the spkg by Daniel Krenn in the first comment and then apply the patch.
comment:11 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:12 Changed 7 years ago by
I think the name (and commit message) of the patch is kind of misleading since you are actually changing code and not just fixing doctests.
Anyway, the code looks good - I'm running make ptestlong
just to make sure.
comment:13 Changed 7 years ago by
I fixed a doctest that was failing (deprecation warnings only appear once per session). I also took the liberty of fixing formatting and trailing whitespace in the functions you touched :)
patchbot: apply trac_12806_doctest_fixes.patch trac_12806.reviewer.patch
comment:14 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Keshav Kini
comment:15 Changed 7 years ago by
If you agree with the reviewer patch, I'll set this to positive review.
comment:16 Changed 7 years ago by
- Status changed from needs_review to positive_review
Thanks for the prompt response, Keshav! The reviewer patch looks good to me, so I guess we can consider this reviewed!
Now it is time for all the graph people to start doing cool stuff with all the great functionalities of NetworkX we have been missing out! :-D
comment:17 Changed 7 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:18 Changed 7 years ago by
- Status changed from positive_review to needs_work
The documentation doesn't build correctly. I've lost the exact warning/error messages though.
comment:19 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
I think the documentation building problem is fixed; sage --docbuild reference pdf
gives me no errors. Needs review just for the docbuild patch.
comment:20 Changed 7 years ago by
patchbot: apply trac_12806_doctest_fixes.patch trac_12806.reviewer.patch trac_12806_docbuild_fix.patch
comment:21 Changed 7 years ago by
I'm not sure what you did, but the third patch does not apply - pretty obviously, it is not based on the reviewer patch. Needs work. I may put something together quickly for this.
comment:22 follow-up: ↓ 23 Changed 7 years ago by
There were a few other build errors, and incorrect linking syntax. I'll fix these.
Also, is
equivalent to ``weight = weight``
supposed to be
equivalent to ``weight = 'weight'``
since weight takes a boolean or a string?
comment:23 in reply to: ↑ 22 Changed 7 years ago by
Thanks for jumping in!
I must have messed up my patch queue at some stage. I am building a clean version of sage right now, will made a new patch on top of it if you don't beat me to it.
Replying to kcrisman:
Also, is
equivalent to ``weight = weight``supposed to be
equivalent to ``weight = 'weight'``since weight takes a boolean or a string?
That's right, nice catch. Thanks again for your help!
comment:24 Changed 7 years ago by
- Description modified (diff)
- Reviewers changed from Keshav Kini to Keshav Kini, Karl-Dieter Crisman
Ok, I didn't see your message, so we'll go with the alt patch.
Patchbot:
Apply trac_12806_doctest_fixes.patch, trac_12806.reviewer.patch and trac_12806-docbuild-and-links-alt.patch
comment:25 Changed 7 years ago by
@jlopez: I'll leave it to you to give final positive review, but this should catch all of the docbuild errors, fix some links, and removed some whitespace that was right next to the stuff being changed anyway.
comment:26 Changed 7 years ago by
- Status changed from needs_review to positive_review
looks good to go.
comment:27 Changed 7 years ago by
- Merged in set to sage-5.2.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
I made a package networkx-1.6, which is available here.
There are a lot of doctests failing in sage.graphs.