Opened 7 years ago

Closed 7 years ago

#16215 closed enhancement (fixed)

Error raised when non-multi(di)graph receive multiple edges as input

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.2
Component: graph theory Keywords: graph, digraph, multiedges, error
Cc: Merged in:
Authors: Vincent Delecroix Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: f6102dd (Commits) Commit: f6102dd6b2fdb2f47c08e093db135a85243a156b
Dependencies: Stopgaps:

Description (last modified by vdelecroix) 

sage: Graph([(0,1),(0,1)],multiedges=False)
Traceback (most recent call last):
...
ValueError: Non-multigraph input dict has multiple edges (0,1)
sage: DiGraph([(0,1),(0,1)],multiedges=False)
Traceback (most recent call last):
...
NameError: global name 'choice' is not defined

The first message is much clearer :-P

Nathann

Change History (12)

comment:1 Changed 7 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by ncohen

  • Branch set to u/ncohen/16215
  • Commit set to 7f50072dc0ab7cd38e5a2af14534b4ce012fa9f2

New commits:

​7f50072trac #16215: A missing import

comment:3 Changed 7 years ago by vdelecroix

  • Description modified (diff)
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

Could you instead replace 

choice([v for v in data[u] if data[u].count(v) > 1])

with 

(v for v in data[u] if data[u].count(v) > 1).next()

or anything similar. The function choice involves some randomness that is useless here.

comment:4 Changed 7 years ago by ncohen

But it's funny... And it has been there from the start, and there is the same in Graph !

Nathann

comment:5 Changed 7 years ago by vdelecroix

It is funny, but it was wrong and nobody noticed... so the simplest, the best. Especially in the code of a complicated constructor (~600 lines for the one in Graph). And as you mentioned, It would be better to remove the one in Graph as well ;-P

Vincent

comment:6 Changed 7 years ago by ncohen

For me the simplest is to add the missing line. If you want to solve it differently, add a commit. There is nothing tricky involved, you do not need me to write it.

Nathann

comment:7 Changed 7 years ago by vdelecroix

  • Authors changed from Nathann Cohen to Vincent Delecroix
  • Branch changed from u/ncohen/16215 to u/vdelecroix/16215
  • Commit changed from 7f50072dc0ab7cd38e5a2af14534b4ce012fa9f2 to f6102dd6b2fdb2f47c08e093db135a85243a156b
  • Keywords graph digraph multiedges error added
  • Reviewers Vincent Delecroix deleted
  • Status changed from needs_work to needs_review
  • Summary changed from A missing "import" in DiGraph to Error raised when non-multi(di)graph receive multiple edges as input

We now have 

sage: Graph([(0,1),(0,1)], multiedges=False)
Traceback (most recent call last):
...
ValueError: Non-multigraph got several edges (0,1)
sage: DiGraph([(0,1),(0,1)], multiedges=False)
Traceback (most recent call last):
...
ValueError: Non-multidigraph got several edges (0,1)

Needs review.

New commits:

​f6102ddError when multiple edges are sent to a Non-multi(di)graph

comment:8 Changed 7 years ago by ncohen

  • Status changed from needs_review to positive_review

All tests pass, good to go !

Nathann

comment:9 Changed 7 years ago by vdelecroix

Thanks !

comment:10 follow-up: Changed 7 years ago by vbraun

Reviewer name

comment:11 in reply to: ↑ 10 Changed 7 years ago by vdelecroix

  • Reviewers set to Nathann Cohen

Replying to vbraun:

Reviewer name

Sorry. Here it is.

comment:12 Changed 7 years ago by vbraun

  • Branch changed from u/vdelecroix/16215 to f6102dd6b2fdb2f47c08e093db135a85243a156b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.