Opened 6 years ago

Closed 6 years ago

#15681 closed defect (fixed)

Name of immutable graphs

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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) 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 6 years ago by ncohen

  • Branch set to u/ncohen/15681
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by git

  • Commit set to 898a8e64f5f96dea06b16d1f9f17b583f75d7fe4

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

0f57805trac #15622: Immutable graphs must not be relabeled
b6ed038trac #15623: Immutable graph backend for Posets
ee8c395Trac 15623: Let to_graph return an immutable graph
dcb8a0btrac #15623: rebase on 6.1.beta4
529f785trac #15622: More informative exception message
2245c02Trac 15622: Review commit, fixing a misspelled doctest
60ad575trac #15622: Rebase on 6.1.beta3
6398780trac #15622: bugfix before #15623 gets merged
3531566trac #15623: Rebase on updated #15622
95cecd1trac #15623: Removing the hack from #15622, update a variable's name

comment:3 Changed 6 years ago by ncohen

  • Cc SimonKing added; SimoonKing removed

comment:4 Changed 6 years ago by tscrim

Hey Nathann,

Could you add a few doctests showing this is fixed and the new behavior?

Thanks,
Travis

comment:5 Changed 6 years ago by git

  • Commit changed from 898a8e64f5f96dea06b16d1f9f17b583f75d7fe4 to 46cfc90f24aacec0d39f5828709117683ea61b06

Branch pushed to git repo; I updated commit sha1. New commits:

46cfc90trac #15681: A doctest for the bugfix

comment:6 Changed 6 years ago by ncohen

I don't know what else to add O_o

Nathann

comment:7 Changed 6 years ago by tscrim

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

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 6 years ago by git

  • Commit changed from 46cfc90f24aacec0d39f5828709117683ea61b06 to 7e9d208f0c1bc4ca3ef237e4bff182ee58e28b82

Branch pushed to git repo; I updated commit sha1. New commits:

5620ec5trac #15681: Rebase on6.1.beta5
7e9d208trac #15681: New doctest for name() on immutable graphs

comment:10 Changed 6 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

*shrugs* Positive review then. Thanks Nathann!

comment:11 follow-up: Changed 6 years ago by ncohen

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 6 years ago by tscrim

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 6 years ago by tscrim

  • Branch changed from u/ncohen/15681 to u/tscrim/ticket/15681
  • Commit changed from 7e9d208f0c1bc4ca3ef237e4bff182ee58e28b82 to 81fc8a472a2636acfaeaf132b085878f5ec3674f

Branch with rebased #15623.


New commits:

dae53afMerge branch 'develop' into ticket/15623
c5b6d59Re-insert a bit of code that had been erroneously deleted in the previous merge commit
81fc8a4Merge branch 'u/ncohen/15681' of trac.sagemath.org:sage into u/tscrim/15681

comment:14 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.