Opened 17 months ago
Closed 17 months ago
#32478 closed defect (fixed)
trivial __copy__ and __deepcopy__ methods for number field elements
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  number fields  Keywords:  
Cc:  tscrim, nbruin, klee, vdelecroix, mmezzarobba  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e971073 (Commits, GitHub, GitLab)  Commit:  e9710736230e25beca57514d3cd527bc63dd457e 
Dependencies:  Stopgaps: 
Description (last modified by )
(split out from #13811, followup on #32454)
Most Sage objects are immutable. Nevertheless, copy
and deepcopy
make copies (through pickling/unpickling) for them because we have not provided the classes with __copy__
methods (which should just return the object) and __deepcopy__
methods (which in many cases should just return the object).
sage: a = 0 sage: copy(a) is a False
In this ticket, we take care of number field elements.
Change History (14)
comment:1 Changed 17 months ago by
Branch:  → u/mkoeppe/trivial___copy___and___deepcopy___methods_for_number_field_elements 

comment:2 Changed 17 months ago by
Cc:  mmezzarobba added 

Commit:  → 8a8cfdc38fee948661aed8da16606bedd422d848 
comment:3 Changed 17 months ago by
Description:  modified (diff) 

comment:4 Changed 17 months ago by
Commit:  8a8cfdc38fee948661aed8da16606bedd422d848 → 31c393e2544c79c9f3b863c83de1b513c8fcafa1 

Branch pushed to git repo; I updated commit sha1. New commits:
31c393e  sage.rings.number_field.maps.NameChangeMap: Do not rely on __copy__ making an actual copy of an immutable element

comment:5 Changed 17 months ago by
Authors:  → Matthias Koeppe 

Status:  new → needs_review 
The failures came from the defective pattern discussed in https://trac.sagemath.org/ticket/13811#comment:49, fixed now.
comment:6 Changed 17 months ago by
Description:  modified (diff) 

comment:7 Changed 17 months ago by
Commit:  31c393e2544c79c9f3b863c83de1b513c8fcafa1 → dd0644f9e2d6255cc20921895a0ed0da86938388 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dd0644f  sage.rings.number_field.maps.NameChangeMap: Do not rely on __copy__ making an actual copy of an immutable element

comment:8 Changed 17 months ago by
A whileweareatit, we probably should optimize def _copy_for_parent(self, parent):
as
cpdef _copy_for_parent(self, Parent parent):
Otherwise LGTM.
comment:9 Changed 17 months ago by
Commit:  dd0644f9e2d6255cc20921895a0ed0da86938388 → c8124a9905f9bc2ffa406e4772a45a49e99c89ce 

Branch pushed to git repo; I updated commit sha1. New commits:
c8124a9  sage.rings.numberfield: Make _copy_for_parent methods cpdef

comment:10 Changed 17 months ago by
Thank you.
Sorry, there one other doc thing I just noticed. For NumberFieldElement_quadratic.__deepcopy__()
, the documentation is wrong as it does not return a copy.
comment:11 Changed 17 months ago by
Commit:  c8124a9905f9bc2ffa406e4772a45a49e99c89ce → e9710736230e25beca57514d3cd527bc63dd457e 

Branch pushed to git repo; I updated commit sha1. New commits:
e971073  NumberFieldElement_quadratic.__copy__, __deepcopy__: Remove misleading doc

comment:12 Changed 17 months ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
Thank you. LGTM.
comment:14 Changed 17 months ago by
Branch:  u/mkoeppe/trivial___copy___and___deepcopy___methods_for_number_field_elements → e9710736230e25beca57514d3cd527bc63dd457e 

Resolution:  → fixed 
Status:  positive_review → closed 
As mentioned in https://trac.sagemath.org/ticket/13811#comment:37, the change for
NumberFieldElement
leads to doctest errors.New commits:
NumberFieldElement_quadratic.__copy__, __deepcopy__: Immutable, so just return self
NumberFieldElement.__copy__, __deepcopy__: Immutable, so just return self