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

memoryview doesn't allow tuple-indexing #67820

Closed
pitrou opened this issue Mar 10, 2015 · 18 comments
Closed

memoryview doesn't allow tuple-indexing #67820

pitrou opened this issue Mar 10, 2015 · 18 comments
Labels
type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Mar 10, 2015

BPO 23632
Nosy @pitrou, @bitdancer, @skrah, @serhiy-storchaka
Files
  • memoryview_tuple_indexing.patch
  • memoryview_tuple_indexing2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-03-19.23:11:26.897>
    created_at = <Date 2015-03-10.21:46:41.107>
    labels = ['type-feature']
    title = "memoryview doesn't allow tuple-indexing"
    updated_at = <Date 2015-03-19.23:11:26.895>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2015-03-19.23:11:26.895>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-03-19.23:11:26.897>
    closer = 'pitrou'
    components = []
    creation = <Date 2015-03-10.21:46:41.107>
    creator = 'pitrou'
    dependencies = []
    files = ['38431', '38550']
    hgrepos = []
    issue_num = 23632
    keywords = ['patch']
    message_count = 18.0
    messages = ['237814', '237820', '237926', '237927', '237928', '237930', '237931', '237932', '237933', '237934', '237935', '238093', '238471', '238475', '238479', '238492', '238580', '238581']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'r.david.murray', 'skrah', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue23632'
    versions = ['Python 3.5']

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 10, 2015

    It is a bit of pity. A least non-sliced indexing should be able to work:

    >>> import numpy as np
    >>> a = np.arange(10)
    >>> memoryview(a)[0]
    0
    >>> a = np.arange(10).reshape((2,5))
    >>> a[0,1]
    1
    >>> memoryview(a)[0,1]
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: memoryview: invalid slice key

    @pitrou pitrou added the type-feature A feature request or enhancement label Mar 10, 2015
    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 10, 2015

    Here is a patch.

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 12, 2015

    Aren't there wider implications of python starting to support tuple indexing? If we make this work, aren't people going to expect [[1,2], [3,4]][0,1] to work?

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 12, 2015

    If we make this work, aren't people going to expect [[1,2], [3,4]][0,1] to work?

    Or even [[1,2], [3,4]][*(0,1)] :-)

    But seriously, I don't know. memorview is a pretty specialized object, its semantics have more to do with Numpy (which has been a major inspiration and use case for the buffer protocol's design) than standard Python containers. I wouldn't expect a list of lists to support tuple indexing.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 12, 2015

    Multi-dimensional slicing is explicitly mentioned in PEP-3118, so
    I guess the intention was to cover this use case as well.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 12, 2015

    To be sure, the PEP contains some odd proposals like "Unpacking a long-double will return a decimal object", but the one in this issue seems reasonable.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 12, 2015

    May be add support of multi-dimensional sub-views?

    >>> m = memoryview(bytearray(range(12)))
    >>> m2 = m.cast('B', (3,4))
    >>> m2[0]
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    NotImplementedError: multi-dimensional sub-views are not implemented

    Then we could use m2[0][1].

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 12, 2015

    Sub-views are not as easily implemented. Besides, they create an intermediate object which is immediately thrown away. And tuple indexing is a very common idiom in the Numpy universe.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 12, 2015

    sub-views and multi-dimensional slicing are not that bad: They're already implemented in _testbuffer (indeed with intermediate objects though).

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 12, 2015

    Well, we could certainly do both :) I don't think we should weigh this issue with separate features, though.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 12, 2015

    Yes, to be clear I'm +1 on this specific feature -- and separate issues. :)

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 14, 2015

    It turns out that msg237933 was too simplistic: There are some
    intricacies of numpy array indexing that *are* very complex:

    http://docs.scipy.org/doc/numpy/reference/arrays.indexing.html

    In particular, x[::2, 1, ::2] is not necessarily equal to
    x[::2][1][::2].

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 18, 2015

    Do you want to review the patch?

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 18, 2015

    I left few nitpicks on Rietveld, but in general the patch LGTM.

    Oh, needed a documentation.

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 18, 2015

    Updated patch addressing Serhiy's comments.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 19, 2015

    It would be good to add examples with multidimensional memoryviews. For example:

    >>> a = array.array('l', [-11111111, 22222222, -33333333, 44444444])
    >>> m = memoryview(a).cast('B').cast('l', [2, 2])
    >>> m[0, 0]
    -11111111
    >>> m[1, 0]
    -33333333
    >>> m[-1, -1]
    44444444

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 19, 2015

    New changeset 7d4eb5902f82 by Antoine Pitrou in branch 'default':
    Issue bpo-23632: Memoryviews now allow tuple indexing (including for multi-dimensional memoryviews).
    https://hg.python.org/cpython/rev/7d4eb5902f82

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 19, 2015

    I didn't add the 2d example as the double cast is quite ugly. Probably we should allow casting when the format stays the same.

    @pitrou pitrou closed this as completed Mar 19, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants