Opened 8 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 kcrisman)

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:

  1. Fixed the calls in graph.py to conform to the new API for betweennness_centrality. Also exposed the new options.
  2. 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
  3. 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.
  4. 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:

  1. Install this spkg
  2. Apply trac_12806_doctest_fixes.patch, trac_12806.reviewer.patch and trac_12806-docbuild-and-links-alt.patch

Attachments (3)

trac_12806_doctest_fixes.patch (14.5 KB) - added by jlopez 7 years ago.
trac_12806.reviewer.patch (20.1 KB) - added by kini 7 years ago.
apply to $SAGE_ROOT/devel/sage
trac_12806-docbuild-and-links-alt.patch (5.8 KB) - added by kcrisman 7 years ago.
apply if weight='weight'

Download all attachments as: .zip

Change History (30)

comment:1 Changed 8 years ago by dkrenn

  • Authors set to Daniel Krenn

I made a package networkx-1.6, which is available here.

There are a lot of doctests failing in sage.graphs.

comment:2 Changed 8 years ago by kini

  • Cc kini added

comment:3 Changed 8 years ago by fbissey

  • Cc fbissey added

comment:4 Changed 7 years ago by brunellus

  • Cc brunellus added

comment:5 Changed 7 years ago by jlopez

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 jlopez

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 jlopez

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 jlopez

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 ncohen

  • Cc ncohen added

Changed 7 years ago by jlopez

comment:10 Changed 7 years ago by jlopez

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 jlopez

  • Authors changed from Daniel Krenn to Daniel Krenn, Javier López Peña
  • Description modified (diff)
  • Status changed from new to needs_review

comment:12 Changed 7 years ago by kini

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.

Changed 7 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:13 Changed 7 years ago by kini

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 kini

  • Description modified (diff)
  • Reviewers set to Keshav Kini

comment:15 Changed 7 years ago by kini

If you agree with the reviewer patch, I'll set this to positive review.

comment:16 Changed 7 years ago by jlopez

  • 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 jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:18 Changed 7 years ago by jdemeyer

  • 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 jlopez

  • 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 jlopez

patchbot: apply trac_12806_doctest_fixes.patch trac_12806.reviewer.patch trac_12806_docbuild_fix.patch

comment:21 Changed 7 years ago by kcrisman

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: Changed 7 years ago by kcrisman

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 jlopez

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!

Changed 7 years ago by kcrisman

apply if weight='weight'

comment:24 Changed 7 years ago by kcrisman

  • 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 kcrisman

@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 dimpase

  • Status changed from needs_review to positive_review

looks good to go.

comment:27 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.2.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.