Opened 10 months ago
Closed 9 months ago
#31330 closed enhancement (fixed)
less usage of deepcopy in quadratic forms
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | quadratic forms | Keywords: | |
Cc: | tscrim, slelievre | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | e2e476a (Commits, GitHub, GitLab) | Commit: | e2e476ad8ec83eb1cb5e9d53290cf430b306b0b0 |
Dependencies: | Stopgaps: |
Description
Change History (11)
comment:1 Changed 10 months ago by
- Branch set to u/chapoton/31330
- Commit set to d67b4ea2ea84abe2feca51b072856ac61f2d3cf9
- Status changed from new to needs_review
comment:2 Changed 10 months ago by
- Cc tscrim slelievre added
patchbot looks green enough, please review
comment:3 Changed 10 months ago by
Could you give some justification for this change? Does it result in a speed up?
comment:4 Changed 10 months ago by
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
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)) ~/sage/local/lib/python3.9/site-packages/sage/quadratic_forms/quadratic_form__local_normal_form.py in local_normal_form(self, p) 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 ~/sage/local/lib/python3.9/site-packages/sage/quadratic_forms/quadratic_form__variable_substitutions.py in extract_variables(self, var_indices) 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
- 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
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
- Commit changed from d67b4ea2ea84abe2feca51b072856ac61f2d3cf9 to e2e476ad8ec83eb1cb5e9d53290cf430b306b0b0
comment:9 Changed 9 months ago by
ok, I have now removed the other DIAGNOSTIC blocks
comment:10 Changed 9 months ago by
- 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
- Branch changed from u/chapoton/31330 to e2e476ad8ec83eb1cb5e9d53290cf430b306b0b0
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
less usage of deepcopy in quadratic forms