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
- Branch set to u/vbraun/remove_graphbundle
comment:2 Changed 5 years ago by
- Cc ncohen jason added
- Commit set to bfb444341d3b3a3f3a546338bc61f79085b9abf1
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
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
- Reviewers set to Nathann Cohen
- Status changed from needs_review to positive_review
comment:5 follow-up: ↓ 6 Changed 5 years ago by
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
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
(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
If thats all then I'll be happy to review it...
comment:9 Changed 5 years ago by
- Branch changed from u/vbraun/remove_graphbundle to bfb444341d3b3a3f3a546338bc61f79085b9abf1
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Delete GraphBundle