Opened 8 years ago

Closed 8 years ago

#18028 closed defect (fixed)

Remove GraphBundle

Reported by: Volker Braun Owned by:
Priority: major Milestone: sage-6.6
Component: graph theory Keywords:
Cc: Nathann Cohen, Jason Grout Merged in:
Authors: Volker Braun Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: bfb4443 (Commits, GitHub, GitLab) Commit: bfb444341d3b3a3f3a546338bc61f79085b9abf1
Dependencies: Stopgaps:

Status badges

Description

Doesn't work and nothing changed over the last 5 years. If you want it back its always in the git repo.

Change History (9)

comment:1 Changed 8 years ago by Volker Braun

Branch: u/vbraun/remove_graphbundle

comment:2 Changed 8 years ago by Volker Braun

Authors: Volker Braun
Cc: Nathann Cohen Jason Grout added
Commit: bfb444341d3b3a3f3a546338bc61f79085b9abf1
Status: newneeds_review

New commits:

bfb4443Delete GraphBundle

comment:3 Changed 8 years ago by Nathann Cohen

Wow. Glad to see that it is actually possible to remove code from Sage. I always thought that it was 'politically impossible' to remove anything from this software, but fortunately I seem to be wrong.

What would you think of removing BipartiteGraph while we are at it ? This class' design is awful:

sage: g=BipartiteGraph()
sage: l=[(0,1),(3,2),(1,2),(0,3)]
sage: for e in l:
....:     g.add_edge(e)
RuntimeError: Edge vertices must lie in different partitions.

but

sage: g = Graph()
sage: g.add_edges(l)
sage: g.is_bipartite()
True

Also, I just noticed that one can very easily build a non-bipartite BipartiteGraph::

sage: g = BipartiteGraph()
sage: g.add_edges(graphs.CompleteGraph(30).edges())
sage: g.is_bipartite()
False

Please. Can you help me get rid of that class? I'll write the ticket if you agree.

Nathann

comment:4 Changed 8 years ago by Nathann Cohen

Reviewers: Nathann Cohen
Status: needs_reviewpositive_review

comment:5 Changed 8 years ago by Volker Braun

I've never used BipartiteGraph. Is it actually useful, just with a bad interface? Or completely non-functional? At least you seem to be able to construct one, maybe start with a deprecation and see if anybody complains.

comment:6 in reply to:  5 Changed 8 years ago by Nathann Cohen

I've never used BipartiteGraph. Is it actually useful, just with a bad interface? Or completely non-functional? At least you seem to be able to construct one, maybe start with a deprecation and see if anybody complains.

At some point it was claimed that it was "for teaching purposes", and it was badly designed from the start. add_edge does not check that the graph is bipartite, it checks that the previously computed bipartition stays valid, which is a wrong path to take for non-connected graphs.

Then you have the usual problems:

sage: g=BipartiteGraph(5)
sage: g.complement()
...
RuntimeError: Edge vertices must lie in different partitions.

Honestly if we must have such a class, we cannot afford to let it inherit from Graph. And from the way it appears in the doc, real graph users may want to give it a try, and regret it later.

Nathann

comment:7 Changed 8 years ago by Nathann Cohen

(basically its only 'feature' is to prevent you from making your graph non-bipartite. And given that it does that wrong, it is probably better if the users call .is_bipartite often instead of relying on the (wrong) exceptions raised by add_edge).

Nathann

comment:8 Changed 8 years ago by Volker Braun

If thats all then I'll be happy to review it...

comment:9 Changed 8 years ago by Volker Braun

Branch: u/vbraun/remove_graphbundlebfb444341d3b3a3f3a546338bc61f79085b9abf1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.