Opened 8 years ago
Closed 8 years ago
#15681 closed defect (fixed)
Name of immutable graphs
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  graph theory  Keywords:  
Cc:  SimonKing, tscrim  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  u/tscrim/ticket/15681 (Commits, GitHub, GitLab)  Commit:  81fc8a472a2636acfaeaf132b085878f5ec3674f 
Dependencies:  #15623  Stopgaps: 
Description
Travis noticed in #15669 that immutable graphs were nameless. That's terrible.
sage: graphs.PetersenGraph() Petersen graph: Graph on 10 vertices sage: Graph(graphs.PetersenGraph(),immutable=True) Graph on 10 vertices
This patch fixes it. So doing, we now store the name
attribute in the Python graph itself and not in the backend, where it does not really belong. This thing is for storing the graph data, not fancy attributes !
New behaviour :
sage: graphs.PetersenGraph() Petersen graph: Graph on 10 vertices sage: Graph(graphs.PetersenGraph(),immutable=True) Petersen graph: Graph on 10 vertices
This patch is based upon #15623, because God made it that we will have to pay the Poset's ._immutable
hack until #15623 is merged :P
Nathann
Change History (15)
comment:1 Changed 8 years ago by
 Branch set to u/ncohen/15681
 Status changed from new to needs_review
comment:2 Changed 8 years ago by
 Commit set to 898a8e64f5f96dea06b16d1f9f17b583f75d7fe4
comment:3 Changed 8 years ago by
 Cc SimonKing added; SimoonKing removed
comment:4 Changed 8 years ago by
Hey Nathann,
Could you add a few doctests showing this is fixed and the new behavior?
Thanks,
Travis
comment:5 Changed 8 years ago by
 Commit changed from 898a8e64f5f96dea06b16d1f9f17b583f75d7fe4 to 46cfc90f24aacec0d39f5828709117683ea61b06
Branch pushed to git repo; I updated commit sha1. New commits:
46cfc90  trac #15681: A doctest for the bugfix

comment:6 Changed 8 years ago by
I don't know what else to add O_o
Nathann
comment:7 Changed 8 years ago by
Something like
sage: P = graphs.PetersenGraph() sage: G = P.copy(immutable=True) sage: G.name("blah")  Traceback (most recent call last): ... NotImplementedError: An immutable graph does not change name
Although I think a TypeError
is a better error message because IMO NotImplementedError
are for things which have not been implemented yet. Yet this is only my opinion and I won't hold this ticket up because of that.
comment:8 Changed 8 years ago by
Hellooooooooooo !!!
I rebased it on beta5 and added a doctest. I used a NotImplementedError
here, because Simon made me use NotImplementedError for the add_edge
function of the immutable graph backend when I wanted to use ValueError
. I personally think that it does not matter the slightest. And I take as proof that nobody agrees on what it should be, which means that no standard is good here :P
Nathann
comment:9 Changed 8 years ago by
 Commit changed from 46cfc90f24aacec0d39f5828709117683ea61b06 to 7e9d208f0c1bc4ca3ef237e4bff182ee58e28b82
comment:10 Changed 8 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
*shrugs* Positive review then. Thanks Nathann!
comment:11 followup: ↓ 12 Changed 8 years ago by
It's a pleasure to work when tickets are written and reviewed in a couple of days, really :D
Nathann
comment:12 in reply to: ↑ 11 Changed 8 years ago by
Replying to ncohen:
It's a pleasure to work when tickets are written and reviewed in a couple of days, really
:D
Agreed. XD
comment:13 Changed 8 years ago by
 Branch changed from u/ncohen/15681 to u/tscrim/ticket/15681
 Commit changed from 7e9d208f0c1bc4ca3ef237e4bff182ee58e28b82 to 81fc8a472a2636acfaeaf132b085878f5ec3674f
comment:14 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:15 Changed 8 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
trac #15622: Immutable graphs must not be relabeled
trac #15623: Immutable graph backend for Posets
Trac 15623: Let to_graph return an immutable graph
trac #15623: rebase on 6.1.beta4
trac #15622: More informative exception message
Trac 15622: Review commit, fixing a misspelled doctest
trac #15622: Rebase on 6.1.beta3
trac #15622: bugfix before #15623 gets merged
trac #15623: Rebase on updated #15622
trac #15623: Removing the hack from #15622, update a variable's name