Opened 10 months ago

Closed 9 months ago

# less usage of deepcopy in quadratic forms

Reported by: Owned by: chapoton major sage-9.3 quadratic forms tscrim, slelievre Frédéric Chapoton Travis Scrimshaw N/A e2e476a e2e476ad8ec83eb1cb5e9d53290cf430b306b0b0

### comment:1 Changed 10 months ago by chapoton

• Branch set to u/chapoton/31330
• Commit set to d67b4ea2ea84abe2feca51b072856ac61f2d3cf9
• Status changed from new to needs_review

New commits:

 ​d67b4ea `less usage of deepcopy in quadratic forms`

### comment:2 Changed 10 months ago by chapoton

• Cc tscrim slelievre added

patchbot looks green enough, please review

### comment:3 Changed 10 months ago by tscrim

Could you give some justification for this change? Does it result in a speed up?

### comment:4 Changed 10 months ago by chapoton

I made this change to fix something, but I will have to find exactly what was broken.

Anyway, looks very strange to me to use "deepcopy" to initiate a new element.

### comment:5 Changed 10 months ago by chapoton

ok, here is the failing example in sage9.3.b7:

```sage: def test(N):
....:     M = ma(2).tensor_product(ma(N))
....:     return QuadraticForm(M + transpose(M))
....:
sage: def ma(n):
....:     M = matrix(n,n,1)
....:     for k in range(n - 1):
....:         M[k, k+1] = - 1
....:     return M
....:
sage: T4=test(4)
sage: T4.basis_of_short_vectors()
((1, 1, 0, -1, 0, -1, -2, -2),
(0, 1, 1, 0, 0, 1, 0, -1),
(0, 0, 1, 1, 1, 1, 2, 1),
(1, 1, 1, 1, 1, 1, 1, 1),
(0, 1, 1, 1, 1, 1, 1, 1),
(0, 0, 1, 1, 1, 1, 1, 1),
(0, 0, 0, 1, 1, 1, 1, 1),
(0, 0, 1, 1, 0, 0, 1, 1))
sage: T4.local_normal_form(2)
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-5-a253ea314ec0> in <module>
----> 1 T4.local_normal_form(Integer(2))

267                 if Q[i,j] != 0:
268                     raise RuntimeError("Oops!  The cancellation didn't work properly at entry (" + str(i) + ", " + str(j) + ").")
--> 269         Q_Jordan = Q_Jordan + Q.extract_variables(range(block_size))
270         Q = Q.extract_variables(range(block_size, n))
271

261     """
262     m = len(var_indices)
--> 263     Q = copy.deepcopy(self)
264     Q.__init__(self.base_ring(), m)
265     for i in range(m):

~/sage/local/lib/python3.9/copy.py in deepcopy(x, memo, _nil)
170                     y = x
171                 else:
--> 172                     y = _reconstruct(x, memo, *rv)
173
174     # If is its own copy, don't memoize.

~/sage/local/lib/python3.9/copy.py in _reconstruct(x, memo, func, args, state, listiter, dictiter, deepcopy)
268     if state is not None:
269         if deep:
--> 270             state = deepcopy(state, memo)
271         if hasattr(y, '__setstate__'):
272             y.__setstate__(state)

~/sage/local/lib/python3.9/copy.py in deepcopy(x, memo, _nil)
144     copier = _deepcopy_dispatch.get(cls)
145     if copier is not None:
--> 146         y = copier(x, memo)
147     else:
148         if issubclass(cls, type):

~/sage/local/lib/python3.9/copy.py in _deepcopy_dict(x, memo, deepcopy)
227     y = {}
228     memo[id(x)] = y
--> 229     for key, value in x.items():
230         y[deepcopy(key, memo)] = deepcopy(value, memo)
231     return y

RuntimeError: dictionary keys changed during iteration

```

### comment:6 Changed 10 months ago by tscrim

• Reviewers set to Travis Scrimshaw

I also thought it looked strange, but I was thinking there was a reason for it (i.e., speed). However, by calling the `__init__()`, I don't see where there would be any benefits. There are still some uses of `deepcopy`, making feel a bit inconsistent. Yet, that is something that can be removed later.

One last thing, why did you remove only one block of the diagnostic (commented out) code?

### comment:7 Changed 9 months ago by chapoton

sorry, I have no energy left to do something more here. Would it be good enough as it is ?

### comment:8 Changed 9 months ago by git

• Commit changed from d67b4ea2ea84abe2feca51b072856ac61f2d3cf9 to e2e476ad8ec83eb1cb5e9d53290cf430b306b0b0

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

 ​6bbeb60 `Merge branch 'u/chapoton/31330' in 9.3.b9` ​e2e476a `remove other DIAGNOSTIC blocks`

### comment:9 Changed 9 months ago by chapoton

ok, I have now removed the other DIAGNOSTIC blocks

### comment:10 Changed 9 months ago by tscrim

• Status changed from needs_review to positive_review

Sorry, I lost track of this a bit. Yes, this is fine. Thank you.

### comment:11 Changed 9 months ago by vbraun

• Branch changed from u/chapoton/31330 to e2e476ad8ec83eb1cb5e9d53290cf430b306b0b0
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.