Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple loops and edges are lost upon pickling #10916

Closed
nthiery opened this issue Mar 11, 2011 · 8 comments
Closed

Multiple loops and edges are lost upon pickling #10916

nthiery opened this issue Mar 11, 2011 · 8 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Mar 11, 2011

Title says it all:

sage: H = DiGraph(loops=True, multiedges=True)
sage: H.add_vertex(0)
sage: H.add_edge((0,0,0))
sage: H.add_edge((0,0,1))
sage: H.add_edge((0,1,0))
sage: H.add_edge((0,1,1))
sage: H.edges()
[(0, 0, 0), (0, 0, 1), (0, 1, 0), (0, 1, 1)]

sage: HH = loads(dumps(H))
sage: HH.edges()
sage: [(0, 0, 1), (0, 1, 1)]

CC: @sagetrac-sage-combinat @nathanncohen @sagetrac-brunellus

Component: graph theory

Author: Lukáš Lánský

Reviewer: Nathann Cohen

Merged: sage-5.0.beta3

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

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jan 22, 2012

comment:3

Attachment: trac_10916_pickling_repair.patch.gz

Hellooooooooooo !!

Well, this fixes the bug but it also means that the object that is pickled does not necessarily correspond to the original one, as the new one admits multiple edges ^^;

What about setting multiple_edges = self._multiple_edges as it seems to be defined ? And there is an allow_loops() method for the other argument.

Nathann

@sagetrac-brunellus
Copy link
Mannequin

sagetrac-brunellus mannequin commented Jan 22, 2012

comment:4

Hi! I think this is not the case because the DiGraph object is not used in the resulting tuple -- it is just a device to get all arcs. Maybe I don't understand it correctly, but I assume that allow_multiedges is pickled one level higher, in the SparseGraphBackend object.

sage: loads(dumps(Graph({0: [1]})))
Graph on 2 vertices
sage: loads(dumps(Graph({0: [1,1]})))
Multi-graph on 2 vertices

What do you think?

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jan 23, 2012

comment:5

What do you think?

Quite obviously that I said something stupid :-)

Give me a few seconds to give it a more attentive look :-p

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jan 23, 2012

comment:7

Hmmmm... You are totally right, your patch is good and sound. I do not explain why Sage's source code is written like that, but....
I really need to rewrite a lot of stuff there O_O;;

Thank you for this patch, and sorry for my stupid remark :-)

Nathann

@sagetrac-brunellus
Copy link
Mannequin

sagetrac-brunellus mannequin commented Jan 24, 2012

comment:8

Thanks for the fast review! The fix really looks like something everyone would try without any thinking. :-)

Lukáš

@jdemeyer
Copy link

Reviewer: Nathann Cohen

@jdemeyer
Copy link

Author: Lukáš Lánský

@jdemeyer jdemeyer added this to the sage-5.0 milestone Jan 29, 2012
@jdemeyer
Copy link

jdemeyer commented Feb 6, 2012

Merged: sage-5.0.beta3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants