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

Implement advanced indexing (and mixed basic/advanced) #4638

Open
aselle opened this Issue Sep 28, 2016 · 43 comments

Comments

Projects
None yet
@aselle
Member

aselle commented Sep 28, 2016

NumPy style advanced indexing is documented here http://docs.scipy.org/doc/numpy/reference/arrays.indexing.html#advanced-indexing

We currently support basic indexing http://docs.scipy.org/doc/numpy/reference/arrays.indexing.html#basic-slicing-and-indexing
using StridedSlice.

e.g.

foo = Some tensor
idx1=[1,2,3]
idx2=[3,4,5]
foo[idx1, idx2, 3]

@aselle aselle added the enhancement label Sep 28, 2016

@aselle aselle self-assigned this Sep 28, 2016

@aselle

This comment has been minimized.

Show comment
Hide comment
@aselle

aselle Sep 28, 2016

Member

Broken off of #206

Member

aselle commented Sep 28, 2016

Broken off of #206

@shoyer

This comment has been minimized.

Show comment
Hide comment
@shoyer

shoyer Sep 28, 2016

Member

I mentioned this in #206, but I wanted to remind again that mixed indexing with slices/arrays has some really strange/unpredictable behavior for the order of result axes in NumPy. So it might be better to hold off on implementing that in TensorFlow until we're sure we're doing it right.

Member

shoyer commented Sep 28, 2016

I mentioned this in #206, but I wanted to remind again that mixed indexing with slices/arrays has some really strange/unpredictable behavior for the order of result axes in NumPy. So it might be better to hold off on implementing that in TensorFlow until we're sure we're doing it right.

@aselle

This comment has been minimized.

Show comment
Hide comment
@aselle

aselle Sep 28, 2016

Member

Yes, I am aware of how the mixed advanced/basic indexing is super nonintuitive. I could break the mixed indexing into its own issue.

Member

aselle commented Sep 28, 2016

Yes, I am aware of how the mixed advanced/basic indexing is super nonintuitive. I could break the mixed indexing into its own issue.

@yaroslavvb

This comment has been minimized.

Show comment
Hide comment
@yaroslavvb

yaroslavvb Sep 28, 2016

Contributor

idx1/idx2 in your example could be Tensor objects, right?

Contributor

yaroslavvb commented Sep 28, 2016

idx1/idx2 in your example could be Tensor objects, right?

@Rob-Haslinger-Bose

This comment has been minimized.

Show comment
Hide comment
@Rob-Haslinger-Bose

Rob-Haslinger-Bose Oct 4, 2016

does .10 currently support negative indexing? as in X[:,-1] for example?

Rob-Haslinger-Bose commented Oct 4, 2016

does .10 currently support negative indexing? as in X[:,-1] for example?

@aselle

This comment has been minimized.

Show comment
Hide comment
@aselle

aselle Oct 11, 2016

Member

@yaroslavvb: yes, idx1 andidx2 can be tensor objects. even in what is already implemented for baisc indexing, you can do

a=tf.constant(3); 
b=tf.constant(6);
foo[a:b]

@robsync: negative indices have a bug in 0.10 and work in 0.11rc0

Member

aselle commented Oct 11, 2016

@yaroslavvb: yes, idx1 andidx2 can be tensor objects. even in what is already implemented for baisc indexing, you can do

a=tf.constant(3); 
b=tf.constant(6);
foo[a:b]

@robsync: negative indices have a bug in 0.10 and work in 0.11rc0

@Fenugreek

This comment has been minimized.

Show comment
Hide comment
@Fenugreek

Fenugreek Oct 26, 2016

Contributor

@aselle: I saw in #206 according to your 7/12 comment that lvalue basic indexing has been implemented. But what does that mean? Because whenever I do any kind of lvalue indexing

e.g.

foo = Some tensor
foo[:3] = tf.zeros(3)

I get TypeError: 'Tensor' object does not support item assignment.
Thanks.

Contributor

Fenugreek commented Oct 26, 2016

@aselle: I saw in #206 according to your 7/12 comment that lvalue basic indexing has been implemented. But what does that mean? Because whenever I do any kind of lvalue indexing

e.g.

foo = Some tensor
foo[:3] = tf.zeros(3)

I get TypeError: 'Tensor' object does not support item assignment.
Thanks.

@shoyer

This comment has been minimized.

Show comment
Hide comment
@shoyer

shoyer Oct 26, 2016

Member

@Fenugreek Tensors are immutable. But you can do this sort of thing if foo is a tf.Variable by writing foo[:3].assign(tf.zeros(3))

Member

shoyer commented Oct 26, 2016

@Fenugreek Tensors are immutable. But you can do this sort of thing if foo is a tf.Variable by writing foo[:3].assign(tf.zeros(3))

@danijar

This comment has been minimized.

Show comment
Hide comment
@danijar

danijar Oct 26, 2016

Member

...and making sure that it gets executed:

with tf.control_dependencies([foo[:3].assign(tf.zeros(3))]):
    foo = tf.identity(foo)
Member

danijar commented Oct 26, 2016

...and making sure that it gets executed:

with tf.control_dependencies([foo[:3].assign(tf.zeros(3))]):
    foo = tf.identity(foo)
@Fenugreek

This comment has been minimized.

Show comment
Hide comment
@Fenugreek

Fenugreek Oct 26, 2016

Contributor

Great, thanks @shoyer and @danijar . I am past that problem now, but have another error with the basic indexing:

        print pool.get_shape(), args.get_shape(), values.get_shape()
        tf.assign(pool[args], values)

gives

(1024000,) (512000,) (512000,)
Traceback (most recent call last):
...
  File "/usr/local/lib/python2.7/dist-packages/tensorflow/python/ops/array_ops.py", line 1644, in _DelegateStridedSliceShape
    return common_shapes.call_cpp_shape_fn(op, input_tensors_needed=[1, 2, 3])
  File "/usr/local/lib/python2.7/dist-packages/tensorflow/python/framework/common_shapes.py", line 596, in call_cpp_shape_fn
    raise ValueError(err.message)
ValueError: Shape must be rank 1 but is rank 2

There are no rank 2 shapes anywhere, so I am confused. The same error results even if I just do a return pool[args], so has nothing to do with the assignment but with the basic indexing itself. Thanks again.

Contributor

Fenugreek commented Oct 26, 2016

Great, thanks @shoyer and @danijar . I am past that problem now, but have another error with the basic indexing:

        print pool.get_shape(), args.get_shape(), values.get_shape()
        tf.assign(pool[args], values)

gives

(1024000,) (512000,) (512000,)
Traceback (most recent call last):
...
  File "/usr/local/lib/python2.7/dist-packages/tensorflow/python/ops/array_ops.py", line 1644, in _DelegateStridedSliceShape
    return common_shapes.call_cpp_shape_fn(op, input_tensors_needed=[1, 2, 3])
  File "/usr/local/lib/python2.7/dist-packages/tensorflow/python/framework/common_shapes.py", line 596, in call_cpp_shape_fn
    raise ValueError(err.message)
ValueError: Shape must be rank 1 but is rank 2

There are no rank 2 shapes anywhere, so I am confused. The same error results even if I just do a return pool[args], so has nothing to do with the assignment but with the basic indexing itself. Thanks again.

@aselle aselle added type:feature and removed enhancement labels Feb 9, 2017

@charlienash

This comment has been minimized.

Show comment
Hide comment
@charlienash

charlienash Feb 17, 2017

I'm having a similar error to @Fenugreek:

A = tf.constant([[1,2],[3,4],[5,6]])
id_rows = tf.constant([0,2])
A[id_rows, :]

gives an error:

ValueError: Shape must be rank 1 but is rank 2 for 'strided_slice' 
(op: 'StridedSlice') with input shapes: [3,2], [1,2], [1,2], [1].

This is using version 1.0.0. Thanks.

charlienash commented Feb 17, 2017

I'm having a similar error to @Fenugreek:

A = tf.constant([[1,2],[3,4],[5,6]])
id_rows = tf.constant([0,2])
A[id_rows, :]

gives an error:

ValueError: Shape must be rank 1 but is rank 2 for 'strided_slice' 
(op: 'StridedSlice') with input shapes: [3,2], [1,2], [1,2], [1].

This is using version 1.0.0. Thanks.

@aselle

This comment has been minimized.

Show comment
Hide comment
@aselle

aselle Mar 8, 2017

Member

You need to manually broadcast it i.e. values.get_shape() must match pool[args].get_shape. This is a limitation in the current implementation.

Member

aselle commented Mar 8, 2017

You need to manually broadcast it i.e. values.get_shape() must match pool[args].get_shape. This is a limitation in the current implementation.

@Kublai-Jing

This comment has been minimized.

Show comment
Hide comment
@Kublai-Jing

Kublai-Jing Apr 4, 2017

@aselle Can you elaborate more on how to manually broadcast to make it work ? Thanks.

Kublai-Jing commented Apr 4, 2017

@aselle Can you elaborate more on how to manually broadcast to make it work ? Thanks.

@shoyer

This comment has been minimized.

Show comment
Hide comment
@shoyer

shoyer Apr 4, 2017

Member
Member

shoyer commented Apr 4, 2017

@cancan101

This comment has been minimized.

Show comment
Hide comment
Contributor

cancan101 commented Jun 28, 2017

@cancan101

This comment has been minimized.

Show comment
Hide comment
@cancan101

cancan101 Jun 28, 2017

Contributor

@itsmeolivia why was the issue closed? It doesn't seem like the example in the OP works.

Contributor

cancan101 commented Jun 28, 2017

@itsmeolivia why was the issue closed? It doesn't seem like the example in the OP works.

@itsmeolivia

This comment has been minimized.

Show comment
Hide comment
@itsmeolivia

itsmeolivia Jun 28, 2017

Contributor

This question is better asked on StackOverflow since it is not a bug or feature request. There is also a larger community that reads questions there. Thanks!

Contributor

itsmeolivia commented Jun 28, 2017

This question is better asked on StackOverflow since it is not a bug or feature request. There is also a larger community that reads questions there. Thanks!

@cancan101

This comment has been minimized.

Show comment
Hide comment
@cancan101

cancan101 Jun 28, 2017

Contributor

I'm asking why the ticket was closed given the op was in fact a feature. Was it implemented? Or decided to not implement. There is no info in the ticket as to why it was closed.

Contributor

cancan101 commented Jun 28, 2017

I'm asking why the ticket was closed given the op was in fact a feature. Was it implemented? Or decided to not implement. There is no info in the ticket as to why it was closed.

@brianwa84

This comment has been minimized.

Show comment
Hide comment
@brianwa84

brianwa84 Jul 6, 2017

Contributor

@aselle probably want to keep this open right?

Contributor

brianwa84 commented Jul 6, 2017

@aselle probably want to keep this open right?

@danijar danijar reopened this Jul 6, 2017

@SpinachR

This comment has been minimized.

Show comment
Hide comment
@SpinachR

SpinachR Aug 8, 2017

I have realize the advanced indexing like the same way in numpy. Please refer to:
https://github.com/SpinachR/ubuntuTest/blob/master/beautifulCodes/tensorflow_advanced_index_slicing.ipynb

SpinachR commented Aug 8, 2017

I have realize the advanced indexing like the same way in numpy. Please refer to:
https://github.com/SpinachR/ubuntuTest/blob/master/beautifulCodes/tensorflow_advanced_index_slicing.ipynb

@gibiansky

This comment has been minimized.

Show comment
Hide comment
@gibiansky

gibiansky Aug 25, 2017

Contributor

Is there any plan to implement this in TensorFlow?

Contributor

gibiansky commented Aug 25, 2017

Is there any plan to implement this in TensorFlow?

@fword

This comment has been minimized.

Show comment
Hide comment
@fword

fword Nov 13, 2017

any progress?

fword commented Nov 13, 2017

any progress?

@yaroslavvb

This comment has been minimized.

Show comment
Hide comment
@yaroslavvb
Contributor

yaroslavvb commented Nov 13, 2017

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Dec 22, 2017

Member

It has been 14 days with no activity and this issue has an assignee.Please update the label and/or status accordingly.

Member

tensorflowbutler commented Dec 22, 2017

It has been 14 days with no activity and this issue has an assignee.Please update the label and/or status accordingly.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Jan 5, 2018

Member

Nagging Assigneee: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

Member

tensorflowbutler commented Jan 5, 2018

Nagging Assigneee: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Jan 24, 2018

Member

Nagging Assignee: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

Member

tensorflowbutler commented Jan 24, 2018

Nagging Assignee: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@yifeif

This comment has been minimized.

Show comment
Hide comment
@yifeif

yifeif Feb 8, 2018

Member

@aselle do you know if we have plan to implement this? If not, should we mark this as contribution welcome?

Member

yifeif commented Feb 8, 2018

@aselle do you know if we have plan to implement this? If not, should we mark this as contribution welcome?

@asimshankar

This comment has been minimized.

Show comment
Hide comment
@asimshankar
Member

asimshankar commented Feb 8, 2018

@traveller59

This comment has been minimized.

Show comment
Hide comment
@traveller59

traveller59 Feb 9, 2018

use gather to do advanced index need much more code than numpy/pytorch, and make code hard to read. please implement this if possible.
I have implemented a simple function to do some numpy-style advanced indexing based on tf.gather_nd and tf.transpose.

traveller59 commented Feb 9, 2018

use gather to do advanced index need much more code than numpy/pytorch, and make code hard to read. please implement this if possible.
I have implemented a simple function to do some numpy-style advanced indexing based on tf.gather_nd and tf.transpose.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Mar 3, 2018

Member

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

Member

tensorflowbutler commented Mar 3, 2018

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Mar 17, 2018

Member

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

Member

tensorflowbutler commented Mar 17, 2018

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Apr 1, 2018

Member

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

Member

tensorflowbutler commented Apr 1, 2018

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Apr 16, 2018

Member

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

Member

tensorflowbutler commented Apr 16, 2018

Nagging Assignee @aselle: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@aselle

This comment has been minimized.

Show comment
Hide comment
@aselle

aselle Apr 25, 2018

Member

@saxenasaurabh, could you take this issue over?

Member

aselle commented Apr 25, 2018

@saxenasaurabh, could you take this issue over?

@aselle

This comment has been minimized.

Show comment
Hide comment
@aselle

aselle Apr 25, 2018

Member

@saxenasaurabh, could you please take this issue over?

Member

aselle commented Apr 25, 2018

@saxenasaurabh, could you please take this issue over?

@aselle aselle assigned saxenasaurabh and unassigned aselle Apr 25, 2018

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler May 10, 2018

Member

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

Member

tensorflowbutler commented May 10, 2018

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler May 27, 2018

Member

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

Member

tensorflowbutler commented May 27, 2018

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Jun 10, 2018

Member

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

Member

tensorflowbutler commented Jun 10, 2018

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Jun 25, 2018

Member

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

Member

tensorflowbutler commented Jun 25, 2018

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

@tensorflowbutler

This comment has been minimized.

Show comment
Hide comment
@tensorflowbutler

tensorflowbutler Jul 10, 2018

Member

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

Member

tensorflowbutler commented Jul 10, 2018

Please remove the assignee, as this issue is inviting external contributions. Otherwise, remove the contributions welcome label. Thank you.

@gokul-uf

This comment has been minimized.

Show comment
Hide comment
@gokul-uf

gokul-uf Jul 26, 2018

@aselle Is there active development on this front or is using tf Eager the less painful way to go?

Thanks!

gokul-uf commented Jul 26, 2018

@aselle Is there active development on this front or is using tf Eager the less painful way to go?

Thanks!

@asimshankar

This comment has been minimized.

Show comment
Hide comment
@asimshankar

asimshankar Jul 26, 2018

Member

@gokul-uf - Nope, this is not being actively developed at this time. Not quite sure if using eager helps you either. You could easily convert a Tensor to numpy and use advanced indexing, but if you want to compute gradients through that indexing, that won't work.

Member

asimshankar commented Jul 26, 2018

@gokul-uf - Nope, this is not being actively developed at this time. Not quite sure if using eager helps you either. You could easily convert a Tensor to numpy and use advanced indexing, but if you want to compute gradients through that indexing, that won't work.

@shoyer

This comment has been minimized.

Show comment
Hide comment
@shoyer

shoyer Jul 26, 2018

Member
Member

shoyer commented Jul 26, 2018

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