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:

Status badges

Description


Change History (11)

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:

d67b4ealess 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))

~/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 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:

6bbeb60Merge branch 'u/chapoton/31330' in 9.3.b9
e2e476aremove 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.