Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24384 closed enhancement (fixed)

py3: a few changes about range

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: Travis Scrimshaw, Jeroen Demeyer, Erik Bray Merged in:
Authors: Frédéric Chapoton, Travis Scrimshaw Reviewers: Travis Scrimshaw, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 3725ea9 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

as part of #16081

Change History (15)

comment:1 Changed 5 years ago by Frédéric Chapoton

Branch: u/chapoton/24384
Commit: 3b8e35a5eabfb40ecde88bde26f9ae6836d89d4a
Status: newneeds_review

New commits:

3b8e35apy3: again some care for range

comment:2 Changed 5 years ago by Frédéric Chapoton

Cc: Travis Scrimshaw Jeroen Demeyer Erik Bray added

green bot, please review

comment:3 Changed 5 years ago by Travis Scrimshaw

So the changes to tensor_operations.py would not be necessary if we add range to the known Python classes handled by cartesian_product:

-native_python_containers   = set([tuple, list, set, frozenset])
+native_python_containers   = set([tuple, list, set, frozenset, range])

It would be good if we could somehow add all possible views to this (and other similar places for previously-list objects), but range is the most common one that will be passed for this construction.

I also do not see the need to any changes in abvar.py. In Python3 (3.5.2 to be specific), I can do

>>> x = range(5)
>>> x[-2]
3
>>> list(x)
[0, 1, 2, 3, 4]

so i = rows[-1][-1]+1 should not be a problem. Additionally, matrix_from_rows would just convert the range into a list (although I think it probably should not do any such conversion). Am I missing something there?

comment:4 Changed 5 years ago by git

Commit: 3b8e35a5eabfb40ecde88bde26f9ae6836d89d4a98f3c45b80c3beb6eecf5344a74bd448f10e9afe

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

98f3c45py3: again some care for range

comment:5 Changed 5 years ago by Frédéric Chapoton

I have undone the changes to abvar.


New commits:

98f3c45py3: again some care for range

comment:6 Changed 5 years ago by Frédéric Chapoton

green bot. Would it be ok like that for the moment ?

comment:7 Changed 5 years ago by Travis Scrimshaw

Authors: Frédéric ChapotonFrédéric Chapoton, Travis Scrimshaw
Branch: u/chapoton/24384public/python3/cartesian_product_range-24384
Commit: 98f3c45b80c3beb6eecf5344a74bd448f10e9afe3725ea9322659a01c284bbccc4aa57774dccf33a
Reviewers: Travis Scrimshaw, Frédéric Chapoton

I ended up doing the changes to cartesian_product to make that handle a range input as that is something we will want to do eventually and it means we do not have to add a bunch of list wrappers.


New commits:

03f94dcHandle Python3 range in cartesian_product.
3725ea9Porting other changes to tensor_operations.py.

comment:8 Changed 5 years ago by Frédéric Chapoton

Status: needs_reviewpositive_review

ok, thanks

comment:9 Changed 5 years ago by Erik Bray

I think changes like this should also include native iterables that were previously functions that returned lists, such as map and filter as well. Or maybe arbitrary iterables (or at least finite iterables as in isinstance(x, Iterable) and isinstance(x, Sized). The only problem I've found with this is sometimes it's necessary to take care between specific Sage objects that happen to be iterable, and arbitrary/unknown iterables that should be converted to a list.

comment:10 Changed 5 years ago by Erik Bray

Other examples include containers like dict_keys, dict_values, and dict_items.

comment:11 in reply to:  9 Changed 5 years ago by Erik Bray

Replying to embray:

I think changes like this should also include native iterables that were previously functions that returned lists, such as map and filter as well. Or maybe arbitrary iterables (or at least finite iterables as in isinstance(x, Iterable) and isinstance(x, Sized). The only problem I've found with this is sometimes it's necessary to take care between specific Sage objects that happen to be iterable, and arbitrary/unknown iterables that should be converted to a list.

Actually, maybe not Sized since a map is not Sized, and there's no (Pythonic) way to get from a map object what its underlying iterators are, so if it were a map of an infinite iterator you run the risk of blowing up, but I guess that's a problem in general as well.

comment:12 Changed 5 years ago by Erik Bray

Would a test like isinstance(x, Iterable) and not isinstance(x, SageObject) be good enough for most cases of "arbitrary iterable that is not otherwise meaningful to Sage"?

comment:13 Changed 5 years ago by Travis Scrimshaw

I wish those Python3 containers all have some nice common base class, but they don't. :/

Unfortunately, I'm not sure your suggestion in comment:12 would not work because generators are iterable:

sage: g = (x for x in ZZ)
sage: isinstance(g, collections.Iterable) and not isinstance(g, SageObject)
True

So there is no way to guarantee finiteness (right now, I this might be guaranteed for Python object input). Nor do we want strings to be such an object. Granted, we might be able to refine this in a reasonable way, but it will likely be a bit more involved (IDK how much from a quick look).

comment:14 Changed 5 years ago by Volker Braun

Branch: public/python3/cartesian_product_range-243843725ea9322659a01c284bbccc4aa57774dccf33a
Resolution: fixed
Status: positive_reviewclosed

comment:15 Changed 5 years ago by Erik Bray

Commit: 3725ea9322659a01c284bbccc4aa57774dccf33a

I'm also annoyed that there isn't a standard ABC in Python for a finite iteratable. We could make our own though. The problem is that you can't just say it has to have __len__ because some iterables like map aren't necessarily finite, but they can be if the iterable they wrap is finite.

So since there's no way to know absolutely for sure if an iterable is finite I would propose to not worry about it, and if the user provides an infinite iterable that's just as much user error as writing an infinite while loop. It seems to be the only way really, not that I'm happy about it.

Note: See TracTickets for help on using tickets.