#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 mkoeppe)

(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 mkoeppe

Branch: u/mkoeppe/trivial___copy___and___deepcopy___methods_for_number_field_elements

comment:2 Changed 17 months ago by mkoeppe

Cc: mmezzarobba added
Commit: 8a8cfdc38fee948661aed8da16606bedd422d848

As mentioned in https://trac.sagemath.org/ticket/13811#comment:37, the change for NumberFieldElement leads to doctest errors.


New commits:

3791864NumberFieldElement_quadratic.__copy__, __deepcopy__: Immutable, so just return self
8a8cfdcNumberFieldElement.__copy__, __deepcopy__: Immutable, so just return self

comment:3 Changed 17 months ago by mkoeppe

Description: modified (diff)

comment:4 Changed 17 months ago by git

Commit: 8a8cfdc38fee948661aed8da16606bedd422d84831c393e2544c79c9f3b863c83de1b513c8fcafa1

Branch pushed to git repo; I updated commit sha1. New commits:

31c393esage.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 mkoeppe

Authors: Matthias Koeppe
Status: newneeds_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 mkoeppe

Description: modified (diff)

comment:7 Changed 17 months ago by git

Commit: 31c393e2544c79c9f3b863c83de1b513c8fcafa1dd0644f9e2d6255cc20921895a0ed0da86938388

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

dd0644fsage.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 tscrim

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.

Last edited 17 months ago by tscrim (previous) (diff)

comment:9 Changed 17 months ago by git

Commit: dd0644f9e2d6255cc20921895a0ed0da86938388c8124a9905f9bc2ffa406e4772a45a49e99c89ce

Branch pushed to git repo; I updated commit sha1. New commits:

c8124a9sage.rings.numberfield: Make _copy_for_parent methods cpdef

comment:10 Changed 17 months ago by tscrim

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 git

Commit: c8124a9905f9bc2ffa406e4772a45a49e99c89cee9710736230e25beca57514d3cd527bc63dd457e

Branch pushed to git repo; I updated commit sha1. New commits:

e971073NumberFieldElement_quadratic.__copy__, __deepcopy__: Remove misleading doc

comment:12 Changed 17 months ago by tscrim

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Thank you. LGTM.

comment:13 Changed 17 months ago by mkoeppe

Thanks!

comment:14 Changed 17 months ago by vbraun

Branch: u/mkoeppe/trivial___copy___and___deepcopy___methods_for_number_field_elementse9710736230e25beca57514d3cd527bc63dd457e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.