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

EnumeratedSets: Add method 'tuple', avoid making copies #34400

Closed
mkoeppe opened this issue Aug 20, 2022 · 24 comments
Closed

EnumeratedSets: Add method 'tuple', avoid making copies #34400

mkoeppe opened this issue Aug 20, 2022 · 24 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 20, 2022

We change the attribute _list to actually store a tuple of the elements and switch methods such as _iterator_from_list to use the new tuple method instead of going through list (which makes a copy every time).

sage: L = list(range(100))
sage: %timeit list(L)
341 ns ± 5.13 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
sage: %timeit tuple(L)
320 ns ± 9.19 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
sage: T = tuple(range(100))
sage: %timeit list(T)
354 ns ± 2.88 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
sage: %timeit tuple(T)
53.5 ns ± 0.764 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

CC: @tscrim @seblabbe @fchapoton @nthiery

Component: combinatorics

Author: Matthias Koeppe

Branch/Commit: d1a6b08

Reviewer: Travis Scrimshaw

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

@mkoeppe mkoeppe added this to the sage-9.7 milestone Aug 20, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 20, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 20, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 20, 2022

New commits:

bf55dafEnumeratedSets.ParentMethods.tuple: New, replace many uses of list

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 20, 2022

Commit: bf55daf

@mkoeppe mkoeppe changed the title EnumeratedSets: Add methods tuple, _tuple_from_iterator EnumeratedSets: Add methods tuple Aug 20, 2022
@mkoeppe mkoeppe changed the title EnumeratedSets: Add methods tuple EnumeratedSets: Add method tuple Aug 20, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 21, 2022

comment:5

Existing classes that define specialized list methods can (but don't have to be) changed to define tuple methods instead to avoid a tiny bit of copying overhead

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2022

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

d573cbfInfiniteEnumeratedSets.ParentMethods.tuple: New
192a5a2src/sage/categories/enumerated_sets.py: Add doc
d62fe4csrc/sage/categories/enumerated_sets.py: Fix _tuple_from_iterator

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2022

Changed commit from bf55daf to d62fe4c

@mkoeppe mkoeppe changed the title EnumeratedSets: Add method tuple EnumeratedSets: Add method 'tuple', avoid making copies Aug 21, 2022
@tscrim
Copy link
Collaborator

tscrim commented Aug 24, 2022

comment:10

Other than adding at least one doctest to the added methods, LGTM. This is a good idea.

@tscrim
Copy link
Collaborator

tscrim commented Aug 24, 2022

Reviewer: Travis Scrimshaw

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 24, 2022

comment:13

Ah right, I'll add some tests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2022

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

e4e834asrc/sage/categories/enumerated_sets.py: Add doc for _tuple_from_iterator, _tuple_from_list

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2022

Changed commit from d62fe4c to e4e834a

@tscrim
Copy link
Collaborator

tscrim commented Aug 24, 2022

comment:15

Thanks. Green bot => positive review.

@tscrim
Copy link
Collaborator

tscrim commented Aug 24, 2022

comment:16

Actually, you missed def tuple(self): in finite_enumerated_sets.py.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 24, 2022

comment:17

Indeed. I think my eyeballs need some rest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2022

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

fe5bb99src/sage/categories/finite_enumerated_sets.py: Add missing doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2022

Changed commit from e4e834a to fe5bb99

@tscrim
Copy link
Collaborator

tscrim commented Aug 25, 2022

comment:19

Thank you.

Trivial detail that you don't have to do, but EXAMPLE:: -> EXAMPLES:: to follow our general practice within Sage.

Green bot => positive review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2022

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

d1a6b08src/sage/categories/finite_enumerated_sets.py: EXAMPLE:: -> EXAMPLES::

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2022

Changed commit from fe5bb99 to d1a6b08

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2022

comment:21

Thanks, done.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2022

comment:22

Bots are green

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

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