Skip to content

Commit

Permalink
trac sagemath#33255: better copy
Browse files Browse the repository at this point in the history
  • Loading branch information
@dcoudert
dcoudert committed Jan 14, 2023
1 parent 634ca63 commit e993d2f
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 82 deletions.
8 changes: 8 additions & 0 deletions src/sage/graphs/bipartite_graph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class BipartiteGraph(Graph):
- ``hash_labels`` -- boolean (default: ``None``); whether to include edge
labels during hashing. This parameter defaults to ``True`` if the graph is
weighted. This parameter is ignored if the graph is mutable.
Beware that trying to hash unhashable labels will raise an error.
.. NOTE::
Expand Down Expand Up @@ -548,6 +549,8 @@ def __init__(self, data=None, partition=None, check=True, hash_labels=None, *arg
if alist_file:
self.load_afile(data)

if hash_labels is None and hasattr(data, '_hash_labels'):
hash_labels = data._hash_labels
self._hash_labels = hash_labels

return
Expand Down Expand Up @@ -579,6 +582,11 @@ def __hash__(self):
Traceback (most recent call last):
...
TypeError: This graph is mutable, and thus not hashable. Create an immutable copy by `g.copy(immutable=True)`
sage: B = BipartiteGraph([(1, 2, {'length': 3})], immutable=True, hash_labels=True)
sage: B.__hash__()
Traceback (most recent call last):
...
TypeError: unhashable type: 'dict'
"""
if self.is_immutable():
# Determine whether to hash edge labels
Expand Down
7 changes: 5 additions & 2 deletions src/sage/graphs/digraph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ class DiGraph(GenericGraph):
- ``hash_labels`` -- boolean (default: ``None``); whether to include edge
labels during hashing. This parameter defaults to ``True`` if the digraph
is weighted. This parameter is ignored if the digraph is mutable.
Beware that trying to hash unhashable labels will raise an error.
- ``vertex_labels`` -- boolean (default: ``True``); whether to allow any
object as a vertex (slower), or only the integers `0,...,n-1`, where `n`
Expand Down Expand Up @@ -846,6 +847,10 @@ def __init__(self, data=None, pos=None, loops=None, format=None,
# weighted, multiedges, loops, verts and num_verts should now be set
self._weighted = weighted

if hash_labels is None and hasattr(data, '_hash_labels'):
hash_labels = data._hash_labels
self._hash_labels = hash_labels

self._pos = copy(pos)

if format != 'DiGraph' or name is not None:
Expand All @@ -859,8 +864,6 @@ def __init__(self, data=None, pos=None, loops=None, format=None,
self._backend = ib
self._immutable = True

self._hash_labels = hash_labels

# Formats

def dig6_string(self):
Expand Down
117 changes: 39 additions & 78 deletions src/sage/graphs/generic_graph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1040,8 +1040,10 @@ def copy(self, weighted=None, data_structure=None, sparse=None, immutable=None,
``"sparse"`` unless anything else is specified.

- ``hash_labels`` -- boolean (default: ``None``); whether to include
edge labels during hashing. This parameter defaults to ``True`` if the
graph is weighted. This parameter is ignored if the graph is mutable.
edge labels during hashing of the copy. This parameter defaults to
``True`` if the graph is weighted. This parameter is ignored when
parameter ``immutable`` is not ``True``.
Beware that trying to hash unhashable labels will raise an error.

.. NOTE::

Expand Down Expand Up @@ -1200,85 +1202,43 @@ def copy(self, weighted=None, data_structure=None, sparse=None, immutable=None,
sage: G.copy()._backend
<sage.graphs.base.sparse_graph.SparseGraphBackend object at ...>

Copying and changing hash_labels parameter::
Copying and changing ``hash_labels`` parameter::

sage: G1 = Graph({0: {1: 'edge label A'}}, immutable=True, hash_labels=False)
sage: G1c = G1.copy(hash_labels=True, immutable=True)
sage: hash(G1)==hash(G1c)
sage: G = Graph({0: {1: 'edge label A'}}, immutable=True, hash_labels=False)
sage: hash(G.copy(hash_labels=True, immutable=True)) == hash(G)
False

sage: hash(G.copy(hash_labels=False, immutable=True)) == hash(G)
True
sage: hash(G.copy(hash_labels=None, immutable=True)) == hash(G)
True
sage: G = Graph({0: {1: 'edge label A'}}, immutable=True, hash_labels=True)
sage: hash(G.copy(hash_labels=True, immutable=True)) == hash(G)
True
sage: hash(G.copy(hash_labels=False, immutable=True)) == hash(G)
False
sage: hash(G.copy(hash_labels=None, immutable=True)) == hash(G)
True
sage: G1 = Graph({0: {1: 'edge label A'}}, immutable=True, hash_labels=False)
sage: G2 = Graph({0: {1: 'edge label B'}}, immutable=True, hash_labels=False)
sage: hash(G1)==hash(G2)
sage: hash(G1) == hash(G2)
True
sage: G1c = G1.copy(hash_labels=True)
sage: G2c = G2.copy(hash_labels=True)
sage: hash(G1c)==hash(G2c)
sage: G1c = G1.copy(hash_labels=True, immutable=True)
sage: G2c = G2.copy(hash_labels=True, immutable=True)
sage: hash(G1c) == hash(G2c)
False

Making sure the .copy behaviour works correctly with hash_labels and immutable in all 54 cases::
sage: for old_immutable in [True, False]:
....: for new_immutable in [True, False, None]:
....: for old_hash_labels in [True, False, None]:
....: for new_hash_labels in [True, False, None]:
....:
....: # make a graph with old_immutable, old_hash_labels
....: G = Graph({0: {1: 'edge label A'}}, immutable=old_immutable, hash_labels=old_hash_labels)
....: old_immutable=G.is_immutable()
....: old_hash_labels=G._hash_labels
....:
....: # copy the graph, passing the overrides
....: G2 = G.copy(immutable=new_immutable, hash_labels=new_hash_labels)
....:
....: if new_immutable is None:
....: # make sure immutability is preserved if we don't update it
....: assert G2.is_immutable() == old_immutable, [old_immutable, new_immutable, old_hash_labels, new_hash_labels]
....: else:
....: # make sure that updating immutability works
....: assert G2.is_immutable() == new_immutable, [old_immutable, new_immutable, old_hash_labels, new_hash_labels]
....:
....: if new_hash_labels is None:
....: # make sure hash_labels is preserved if we don't update it
....: assert G2._hash_labels == old_hash_labels, [old_immutable, new_immutable, old_hash_labels, new_hash_labels]
....: else:
....: # make sure updating hash labels works
....: assert G2._hash_labels == new_hash_labels, [old_immutable, new_immutable, old_hash_labels, new_hash_labels]

"""

# This is an ugly hack but it works.
# This function contains some fairly complex logic
# There is a comment further down that says

### Immutable copy of an immutable graph ? return self !

# The issue being that if we want to change the hash_labels behaviour, then
# returning self is no longer a good option. I'd argue that a copy function
# returning self is always bad behaviour, but that's out of the scope for this ticket.
# Trying to weaken the if statement to include something like

### and (hash_labels is None or (hash_labels==self._use_labels_for_hash()))

# doesn't work, since there is no fallback logic for making
# an immutable copy of an immutable graph, and my attempts at
# implementing one caused other tests to break in different
# bits of the code

# the hack I've used creates a mutable copy of the graph
# and then makes an immutable copy of that one. I think this is a fairly
# inobtrusive implementation, since the function still runs as normally,
# assuming that they pass nothing into hash_labels, and seems to behave
# correctly otherwise.

# However, this is obviously not optimal, and could definitely be improved upon
# by someone who understands the logic better.
if hash_labels is not None:
desired_immutable = self.is_immutable() if immutable is None else immutable
forced_mutable_copy = self.copy(weighted=weighted, data_structure=data_structure, sparse=sparse, immutable=False)
fresh_copy = forced_mutable_copy.copy(weighted=weighted, data_structure=data_structure, sparse=sparse, immutable=desired_immutable)
fresh_copy._hash_labels = hash_labels
return fresh_copy

sage: G = Graph({0: {1: 'edge label A'}}, immutable=True, hash_labels=False)
sage: H = G.copy(hash_labels=True)
sage: H.is_immutable()
False
sage: H._hash_labels
True
sage: I = H.copy(immutable=True)
sage: hash(G) == hash(I)
False
sage: G = Graph({0: {1: 'edge label A'}}, immutable=True, hash_labels=True)
sage: hash(G) == hash(I)
True
"""
# Which data structure should be used ?
if data_structure is not None:
# data_structure is already defined so there is nothing left to do
Expand Down Expand Up @@ -1306,7 +1266,8 @@ def copy(self, weighted=None, data_structure=None, sparse=None, immutable=None,
# Immutable copy of an immutable graph ? return self !
# (if okay for weightedness)
if (self.is_immutable() and
(weighted is None or self._weighted == weighted)):
(weighted is None or self._weighted == weighted) and
(hash_labels is None or self._hash_labels == hash_labels)):
from sage.graphs.base.static_sparse_backend import StaticSparseBackend
if (isinstance(self._backend, StaticSparseBackend) and
(data_structure == 'static_sparse' or data_structure is None)):
Expand All @@ -1320,10 +1281,10 @@ def copy(self, weighted=None, data_structure=None, sparse=None, immutable=None,
data_structure = "sparse"

G = self.__class__(self, name=self.name(), pos=copy(self._pos),
weighted=weighted,
weighted=weighted, hash_labels=hash_labels,
data_structure=data_structure)

attributes_to_copy = ('_assoc', '_embedding', '_hash_labels')
attributes_to_copy = ('_assoc', '_embedding')
for attr in attributes_to_copy:
if hasattr(self, attr):
copy_attr = {}
Expand Down
7 changes: 5 additions & 2 deletions src/sage/graphs/graph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ class Graph(GenericGraph):
- ``hash_labels`` -- boolean (default: ``None``); whether to include edge
labels during hashing. This parameter defaults to ``True`` if the graph is
weighted. This parameter is ignored if the graph is mutable.
Beware that trying to hash unhashable labels will raise an error.
- ``vertex_labels`` -- boolean (default: ``True``); whether to allow any
object as a vertex (slower), or only the integers `0,...,n-1`, where `n`
Expand Down Expand Up @@ -1257,6 +1258,10 @@ def __init__(self, data=None, pos=None, loops=None, format=None,
weighted = False
self._weighted = getattr(self, '_weighted', weighted)

if hash_labels is None and hasattr(data, '_hash_labels'):
hash_labels = data._hash_labels
self._hash_labels = hash_labels

self._pos = copy(pos)

if format != 'Graph' or name is not None:
Expand All @@ -1270,8 +1275,6 @@ def __init__(self, data=None, pos=None, loops=None, format=None,
self._backend = ib
self._immutable = True

self._hash_labels = hash_labels

# Formats

@doc_index("Basic methods")
Expand Down

0 comments on commit e993d2f

Please sign in to comment.