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

Add more-itertools as a standard package #32100

Open
mkoeppe opened this issue Jul 2, 2021 · 6 comments
Open

Add more-itertools as a standard package #32100

mkoeppe opened this issue Jul 2, 2021 · 6 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 2, 2021

https://pypi.org/project/more-itertools/

Then get rid of our homegrown iteration tools:

  • from sage.misc.misc: uniq, _stable_uniq, subsets/powerset

Useful:

CC: @tscrim @orlitzky @slel @yyyyx4

Component: refactoring

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Jul 2, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@orlitzky
Copy link
Contributor

comment:3

I'll change my mind about this if it lets us delete a good bit of custom code, but looking at the given examples:

  • uniq() is trivial and could be deleted anyway. Either list(set(x)) or sorted(set(x)) are idiomatic python.
  • The benefit of _stable_uniq seems to be that it returns an iterator, but the only two uses of it in the tree immediately convert the result to a list. Can they be replaced by list(set(x))?
  • Powerset is a nice one to have, but by itself totals nine lines of code.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 20, 2021

comment:5

Replying to @orlitzky:

I'll change my mind about this if it lets us delete a good bit of custom code

I agree, there needs to be a demonstrated substantial use before we add another dependency

@slel
Copy link
Member

slel commented Jan 4, 2022

comment:6

One example is slicing a list into sublists
of size "ncpu" to feed a parallel function.

For that, we can each time define a custom
"list of lists" function, for instance

lol = lambda lst, sz: [lst[i:i+sz] for i in range(0, len(lst), sz)]

as found 17 times in sage/manifolds
and sage/tensor (try git grep 'lol =').

We could instead use chunked, ichunked,
sliced or isliced from more-itertools.

@orlitzky
Copy link
Contributor

orlitzky commented Jan 4, 2022

comment:7

lol = lambda lst, sz: [lst[i:i+sz] for i in range(0, len(lst), sz)]

Changing that line on its own won't eliminate much code, though. The pattern

lol = lambda lst, sz: [lst[i:i+sz] for i in range(0, len(lst), sz)]
ind_list = [ind for ind in other._comp] # only this line changes
ind_step = max(1, int(len(ind_list)/nproc/2))
local_list = lol(ind_list, ind_step)

is repeated over and over again. If the whole thing were factored out into a helper function, then we'd be left with changing

lol = lambda lst, sz: [lst[i:i+sz] for i in range(0, len(lst), sz)]

into

lol = lambda lst, sz: chunked(lst,sz)

which only saves us a few characters.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 7, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe removed this from the sage-10.0 milestone Mar 16, 2023
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