Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trivial __copy__ and __deepcopy__ methods for number field elements #32478

Closed
mkoeppe opened this issue Sep 5, 2021 · 21 comments
Closed

trivial __copy__ and __deepcopy__ methods for number field elements #32478

mkoeppe opened this issue Sep 5, 2021 · 21 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 5, 2021

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

CC: @tscrim @nbruin @kwankyu @videlec @mezzarobba

Component: number fields

Author: Matthias Koeppe

Branch/Commit: e971073

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32478

@mkoeppe mkoeppe added this to the sage-9.5 milestone Sep 5, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 5, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 5, 2021

comment:2

As mentioned in #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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 5, 2021

Commit: 8a8cfdc

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

Changed commit from 8a8cfdc to 31c393e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

comment:5

The failures came from the defective pattern discussed in #13811 comment:49, fixed now.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

Changed commit from 31c393e to dd0644f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

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

@tscrim
Copy link
Collaborator

tscrim commented Sep 9, 2021

comment:8

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2021

Changed commit from dd0644f to c8124a9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2021

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

c8124a9sage.rings.numberfield: Make _copy_for_parent methods cpdef

@tscrim
Copy link
Collaborator

tscrim commented Sep 10, 2021

comment:10

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2021

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

e971073NumberFieldElement_quadratic.__copy__, __deepcopy__: Remove misleading doc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2021

Changed commit from c8124a9 to e971073

@tscrim
Copy link
Collaborator

tscrim commented Sep 10, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Sep 10, 2021

comment:12

Thank you. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 10, 2021

comment:13

Thanks!

@vbraun
Copy link
Member

vbraun commented Sep 19, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants