Opened 9 years ago

Closed 9 years ago

#8892 closed defect (fixed)

Many doctests fails since the update of NetworkX !

Reported by: ncohen Owned by: ncohen
Priority: major Milestone: sage-4.4.2
Component: graph theory Keywords:
Cc: Merged in: sage-4.4.2.rc0
Authors: Nathann Cohen Reviewers: Minh Van Nguyen, Jason Grout
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Hello everybody !!!

I noticed working on something quite different that many doctests were failing in Sage's graph library because of the recent update of NetworkX... The reason is easy : the default edge label is not "None" anymore but {}. Besides, dictionary are not hashable !!!

This patch fixes it !

Nathann

Attachments (2)

trac_8892.patch (7.8 KB) - added by ncohen 9 years ago.
trac_8892-reviewer.patch (1.7 KB) - added by mvngu 9 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by jason

Apparently now, networkx has moved to having a dictionary of edge attributes, rather than a specific "label". See http://networkx.lanl.gov/reference/api_1.0.html#edge-attributes

I'm not saying we should follow the convention, but it does seem to make sense. Instead of just storing a single value, store a dict of attributes.

Why is it important that dictionaries are not hashable?

comment:2 Changed 9 years ago by ncohen

Because I sometimes stored them as keys of dictionaries. It means I will need to forget to store the label, and just the endpoints. The other detail is that in many algorithms, the labels are used as a weight, and I you will see very often in Sage's code : weight = lambda label : 1 if label is None else label

So all these occurrences need to be replaces to label == {} in this case... Does that mean we should assume labels are ALWAYS dictionaries ? This would mean trouble... Where would we store numerical values for edges then.. a default field ?

Nathann

comment:3 Changed 9 years ago by jason

A little farther down the networkx page listed, we find http://networkx.lanl.gov/reference/api_1.0.html#converting-your-existing-code-to-networkx-1-0, which says that now all algorithms (in Networkx) look for the "weight" edge attribute.

Sounds like that would be a huge change for Sage code, though...

comment:4 Changed 9 years ago by rbeezer

Hi Nathann,

Thanks for uncovering this one. Not sure right now I have a good idea about what should be done.

However, is there a patch to go on this? I'm not seeing one.

Rob

comment:5 Changed 9 years ago by ncohen

Not yet. For the moment, my patch is a nasty one : replaces tests "is None" by "is None or == {}". I thought it may be better to settle on what we want to do with these labels, but I can upload it otherwise (somewhere on another computer at the moment) :-)

Nathann

comment:6 Changed 9 years ago by ncohen

  • Status changed from new to needs_review

Here is a patch that does not make any choice. I replaced the "is None" by "in RR" :-)

The failing docstrings come from GraphViz? ( at least on my computer ) !

Nathann

comment:7 Changed 9 years ago by ncohen

Now also fixes the edge_coloring function from the graph_coloring module, as reported by Minh in #8781

Sorry for that !

Nathann

Changed 9 years ago by ncohen

Changed 9 years ago by mvngu

comment:8 Changed 9 years ago by mvngu

  • Authors set to Nathann Cohen
  • Reviewers set to Minh Van Nguyen

I'm mostly happy with trac_8892.patch. I have attached a reviewer patch that deals with the part I'm not happy with, i.e. fix some typos introduced by the first patch. Apart from myself, anyone is welcome to give a final sign off on this ticket.

comment:9 follow-up: Changed 9 years ago by jason

  • Owner changed from jason, ncohen, rlm to jason

I sign off on your changes. Are you asking someone else to also sign off on the original patch?

comment:10 Changed 9 years ago by jason

  • Owner changed from jason to ncohen

comment:11 Changed 9 years ago by jason

(I didn't apply your changes, but it is clear that they are cosmetic things.)

comment:12 in reply to: ↑ 9 Changed 9 years ago by mvngu

  • Reviewers changed from Minh Van Nguyen to Minh Van Nguyen, Jason Grout
  • Status changed from needs_review to positive_review

Replying to jason:

Are you asking someone else to also sign off on the original patch?

Not really. I'm OK with ncohen's patch.

comment:13 Changed 9 years ago by mvngu

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