"immutable=True" for Graph/Digraph init and copy() #15603

nathanncohen · 2013-12-28T23:54:32Z

As promised on #15278, this ticket adds an 'immutable' keyword to the constructors of Graph/Digraph, and in 'copy()' too.

While I was at it, I tried to clean a bit the 'copy()' method, which now systematically checks its input. It should deal with every situation :-P

And I think I will have to clean the constructors of Graph/Digraph too at some point.

Nathann

Depends on #15278

CC: @simon-king-jena

Component: graph theory

Author: Nathann Cohen

Branch/Commit: u/SimonKing/ticket/15603 @ b741bd4

Reviewer: Simon King

Issue created by migration from https://trac.sagemath.org/ticket/15603

The text was updated successfully, but these errors were encountered:

nathanncohen · 2013-12-29T00:02:15Z

Dependencies: #15278

nathanncohen · 2013-12-29T00:02:15Z

Changed branch from #15278 to u/ncohen/15603

sagetrac-git · 2013-12-29T00:08:42Z

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

`5a4d634`	trac #15603: "immutable=True" for Graph/Digraph `__init__` and copy()
`9df004b`	`rebasing #15278 on 6.1.beta2`
`51d6328`	`Trac 15278: Error messages explain how to create (im)mutable graph copy`
`2fc8a77`	`Make digraphs using the static backend immutable and hashable`
`07bad46`	`Merge branch 'u/ncohen/15491' of git://trac.sagemath.org/sage into ticket/15278`
`126b036`	`Merge branch 'master' into ticket/15278`
`c774057`	`Prepare hash and copy for immutable graphs. Let .weighted() respect mutability.`

sagetrac-git · 2013-12-29T00:08:42Z

Commit: 5a4d634

sagetrac-git · 2013-12-29T11:14:01Z

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

`0dd973a`	trac #15603: "immutable=True" for Graph/Digraph `__init__` and copy()
`9df004b`	`rebasing #15278 on 6.1.beta2`
`51d6328`	`Trac 15278: Error messages explain how to create (im)mutable graph copy`
`2fc8a77`	`Make digraphs using the static backend immutable and hashable`
`07bad46`	`Merge branch 'u/ncohen/15491' of git://trac.sagemath.org/sage into ticket/15278`
`126b036`	`Merge branch 'master' into ticket/15278`
`c774057`	`Prepare hash and copy for immutable graphs. Let .weighted() respect mutability.`

sagetrac-git · 2013-12-29T11:14:01Z

Changed commit from 5a4d634 to 0dd973a

nathanncohen · 2013-12-29T15:43:34Z

comment:5

What do you think ? :-)

Nathann

sagetrac-git · 2013-12-29T15:44:30Z

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

`7860f39`	trac #15603: "immutable=True" for Graph/Digraph `__init__` and copy()
`2525d22`	`Trac 15278: Fix syntax error in doc test`
`fcf9e30`	`Trac 15278: Only graphs that use the static backend are identical with their copy`
`8fc9c94`	`Merge branch 'develop' into ticket/15278`
`51d6328`	`Trac 15278: Error messages explain how to create (im)mutable graph copy`
`2fc8a77`	`Make digraphs using the static backend immutable and hashable`
`07bad46`	`Merge branch 'u/ncohen/15491' of git://trac.sagemath.org/sage into ticket/15278`
`126b036`	`Merge branch 'master' into ticket/15278`
`c774057`	`Prepare hash and copy for immutable graphs. Let .weighted() respect mutability.`

sagetrac-git · 2013-12-29T15:44:30Z

Changed commit from 0dd973a to 7860f39

simon-king-jena · 2013-12-29T16:08:07Z

comment:7

The commit looks good, except for the spaces in front of colons and question marks... Do you add this space in your smileys, too? :- ? ; -)

I think we agreed that we'll use a new ticket to fix sage.combinat's hack with _immutable=True and our counter-hack with type-checking immutability. Of course, I first want to see if the tests pass before giving a positive review.

nathanncohen · 2013-12-29T16:13:03Z

comment:8

Yoooooo !

The commit looks good, except for the spaces in front of colons and question marks... Do you add this space in your smileys, too? :- ? ; -)

Argggggg T_T

Well right now I'm trying to read a spanish book (even though I don't speak a word of it)... At least I don't put their weird reversed question marks in the code :-PPP

I think we agreed that we'll use a new ticket to fix sage.combinat's hack with _immutable=True and our counter-hack with type-checking immutability. Of course, I first want to see if the tests pass before giving a positive review.

Yepyepyep, sounds right.

Nathann

simon-king-jena · 2013-12-29T19:15:11Z

comment:9

All tests pass, but I think I'll add a review commit, a bit later.

simon-king-jena · 2014-01-03T10:05:20Z

comment:10

Things that need to be done (in a review commit): In some place you explain the immutable=... optional argument, but in the example you use data_structure=... only. In some other place, you do this change:

     If the ``data_structure`` is equal to ``"static_sparse"``, then an
     immutable graph results. Note that this does not use the NetworkX data
     structure::
 
-          sage: G_imm = Graph(g, data_structure="static_sparse")
-          sage: H_imm = Graph(g, data_structure="static_sparse")
+          sage: G_imm = Graph(g, immutable=True)
+          sage: H_imm = Graph(g, immutable=True)
           sage: G_imm == H_imm == G == H
           True

Hence, the text is about data_structure, but you remove it from the test.

nathanncohen · 2014-01-03T10:15:17Z

comment:11

"that need to be done" : will that be your review commit, or do you want me to do that ?

Nathann

simon-king-jena · 2014-01-03T10:20:56Z

comment:12

Replying to @nathanncohen:

"that need to be done" : will that be your review commit, or do you want me to do that ?

I wrote "that need to be done (in a review commit)". Hence, I'll do it.

nathanncohen · 2014-01-03T10:22:29Z

comment:13

Oops sorry. I missed that, in my eagerness to see these patches in Sage ^^;

Nathann

simon-king-jena · 2014-01-03T10:28:44Z

comment:14

There is a naked NotImplementedError:

            sage: g = DiGraph(graphs.PetersenGraph(), immutable=True)
            sage: g.add_edge("Hey", "Heyyyyyyy")
            Traceback (most recent call last):
            ...
            NotImplementedError:

Should we care?

simon-king-jena · 2014-01-03T10:32:30Z

comment:15

            sage: g = graphs.PetersenGraph()
            sage: g = DiGraph(g.edges(),immutable=False)
            sage: g.add_edge("Hey", "Heyyyyyyy")
            sage: {g:1}[g]
            TypeError: This graph is mutable, and thus not hashable. Create an immutable copy by `g.copy(data_structure='static_sparse')`

Should the error message mention the other (more intuitive) possibility g.copy(immutable=True)?

nathanncohen · 2014-01-03T10:37:13Z

comment:16

Hmmmm... We could add an empty function for add_edge in the backend indeed. Depending on where the immutable graph comes from, the user may not have any idea of why this function is not implemented. Something like what add_vertex does.

Yes to your other comment about the exception raised by __hash__.

Nathann

simon-king-jena · 2014-01-03T10:39:32Z

comment:17

Since I want this to get over with and since my family plans to have an excursion today, I prefer to only fix the error message raised by __hash__, but leave the NotImplementedError as it is.

nathanncohen · 2014-01-03T10:41:31Z

comment:18

Ahahahah. Okay no prob. I'll fix that later in another patch ;-)

Nathann

simon-king-jena · 2014-01-03T11:15:48Z

Changed branch from u/ncohen/15603 to u/SimonKing/ticket/15603

simon-king-jena · 2014-01-03T11:20:50Z

Reviewer: Simon King

simon-king-jena · 2014-01-03T11:20:50Z

comment:20

I have added a review commit and run all tests in src/sage/graphs without error. Positive review (unless your aren't happy with my review commit or find that surprisingly the new error message appears in other parts of Sage too).

New commits:

`b741bd4`	`Trac 15603: More doctests, nicer error message`

simon-king-jena · 2014-01-03T11:20:50Z

Changed commit from 7860f39 to b741bd4

nathanncohen · 2014-01-03T11:23:20Z

comment:21

NOnono that' s great ! Thanks :-)

Nathann

nathanncohen mannequin added this to the sage-6.1 milestone Dec 28, 2013

nathanncohen mannequin added c: graph theory labels Dec 28, 2013

This comment has been minimized.

Sign in to view

nathanncohen mannequin added the s: needs review label Dec 28, 2013

simon-king-jena added s: positive review and removed s: needs review labels Jan 3, 2014

vbraun removed the s: positive review label Jan 5, 2014

vbraun closed this as completed in 90ac26e Jan 5, 2014

tscrim mentioned this issue Jun 19, 2014

Implement right-angled Artin groups #15137

Closed

nathanncohen mannequin mentioned this issue Jan 5, 2014

Pickling of immutable graphs #15619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"immutable=True" for Graph/Digraph init and copy() #15603

"immutable=True" for Graph/Digraph init and copy() #15603

nathanncohen mannequin commented Dec 28, 2013

This comment has been minimized.

nathanncohen mannequin commented Dec 29, 2013

nathanncohen mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

nathanncohen mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

simon-king-jena commented Dec 29, 2013

nathanncohen mannequin commented Dec 29, 2013

simon-king-jena commented Dec 29, 2013

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

"immutable=True" for Graph/Digraph __init__ and copy() #15603

"immutable=True" for Graph/Digraph __init__ and copy() #15603

Comments

nathanncohen mannequin commented Dec 28, 2013

This comment has been minimized.

nathanncohen mannequin commented Dec 29, 2013

nathanncohen mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

nathanncohen mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

sagetrac-git mannequin commented Dec 29, 2013

simon-king-jena commented Dec 29, 2013

nathanncohen mannequin commented Dec 29, 2013

simon-king-jena commented Dec 29, 2013

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

simon-king-jena commented Jan 3, 2014

nathanncohen mannequin commented Jan 3, 2014

"immutable=True" for Graph/Digraph init and copy() #15603

"immutable=True" for Graph/Digraph init and copy() #15603