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: |
Status badges
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
NumberFieldElementleads to doctest errors.New commits:
NumberFieldElement_quadratic.__copy__, __deepcopy__: Immutable, so just return selfNumberFieldElement.__copy__, __deepcopy__: Immutable, so just return self