Opened 5 years ago

Closed 5 years ago

#18028 closed defect (fixed)

Remove GraphBundle

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

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 5 years ago by vbraun

  • Branch set to u/vbraun/remove_graphbundle

comment:2 Changed 5 years ago by vbraun

  • Authors set to Volker Braun
  • Cc ncohen jason added
  • Commit set to bfb444341d3b3a3f3a546338bc61f79085b9abf1
  • Status changed from new to needs_review

New commits:

bfb4443Delete GraphBundle

comment:3 Changed 5 years ago by ncohen

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 5 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

comment:5 follow-up: Changed 5 years ago by vbraun

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 5 years ago by ncohen

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 5 years ago by ncohen

(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 5 years ago by vbraun

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

comment:9 Changed 5 years ago by vbraun

  • Branch changed from u/vbraun/remove_graphbundle to bfb444341d3b3a3f3a546338bc61f79085b9abf1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.