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: | sage-9.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, follow-up 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 while-we-are-at-it, 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