Indexing API

[rnett]

Fixes #164.

One question: Should I also make endpoints named get and set? It's more discoverable to people coming from Python (who usually use [] rather than the stridedSlice op).

There's examples in the Index javadoc.

[Craigacp]

More generally these indices don't seem to link up with the indexing operations already available in ndarray, which might prove more confusing as the ndarrays move closer to Tensors given Karl's current refactorings.

[rnett]

I looked at that, there's a few concepts that are impossible to map into stridedSlice, which as far as I know is the only Op we could use for this. In particular, seq and hyperslab. The ndarray API also has no ellipses or newaxis.

If we do want to unify APIs, I think it would be easier to map this API onto the current ndarray one the visa versa. To map the ndarray indices onto Operands we'd either have to remove index options, throw runtime exceptions for unsupported ones, or have a TensorIndex or similar sub-interface. If I was doing it I'd probably have a shared Index interface with TensorIndex and NDArrayIndex for the unsupported ones. Although it seems like you could add operations to the ndarray indexing.

[Craigacp]

I think extending the ndarray API to cover the gaps would be worthwhile, and then we can throw exceptions for things which fail to map from ndarray into TF ops.

[rnett]

There's some odd things in the ndarray API like At not supporting negative indices, Range not having a stride or supporting negative indices, and having flip, even, and odd separate from [::-1], [::2], and [1::2] (although I like having functions for them, I don't think they need to be their own classes). Do you think I should bring the ndarray indexing a bit closer to Numpy? It would make the differences between the ndarray indexing and the tensor indexing less confusing.

[karllessard]

Do you think I should bring the ndarray indexing a bit closer to Numpy? It would make the differences between the ndarray indexing and the tensor indexing less confusing.

Yes, if something is lacking in the NdArray API, it should be added instead of trying to fill the gaps from TensorFlow Java core instead.

If we do want to unify APIs, I think it would be easier to map this API onto the current ndarray one the visa versa. To map the ndarray indices onto Operands...

That was the original intent, we are just not there yet. Again (sorry for that), the tensor type refactoring currently in progress might be helpful to achieve this. I also thing that TType should extend from Operand at some point, so we can pass a tensor directly to an op without converting it to a constant explicitly. I did something like that in a previous draft for the same refactoring here.

Should we also put this PR on hold for now and continue that discussion in #164 ?

[rnett]

I also think that TType should extend from Operand at some point

That would be very nice, especially for eager mode, although I'd have to be careful w/ the names for indexing functions (Operand will always be stridedSlice, but NDArray should be something that makes it obvious it's on the NDArray).

I pushed what I have as far as adding to the ndarray API (it's WIP), it doesn't seem that bad, just newaxis, ellipsis, and strided slice. My impression from the refactoring is that Operand indexing will still have to use StridedSlice? If so most of this PR is still fine, I just need to move the builders to the NDArray builders and unify the APIs (which is mostly done as of the latest commit, just needs cleanup). I'll add a comment to #164 about the API unification changes I would like to make.

Depending on the type refactorings you are working on, it seems like once the APIs are merged, we could merge this without running afoul of the upcoming changes.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[karllessard]

Idem than #167 (comment)

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[rnett]

@karllessard can you re-open this PR too? I don't have the permissions to.

[karllessard]

I'm really sorry, I don't know why but it does not allow me to do it neither, though I was able to do it with #167 ...

[karllessard]

Can you try this? https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c

[rnett]

@karllessard I still have no re-open option, but I did the steps there. Can you do it now? I never had the option, even greyed out, I suspect it's permissions in addition to the force push.

[rnett]

Ok, this is ready for review. One last thing: I'd like to move the builders from Indices to Index, it reads better to me (kind of like if they were nested classes) and is shorter. Thoughts?

[rnett]

Before you merge, any more thoughts on Indices vs Index?

[karllessard]

Yeah I was thinking about that... while I do prefer to move static creators to Index, it is less coherent with the rest of the API like I've explained earlier.

I always had in mind that Indices.* are good candidates to be imported statically. I know that most IDE might complain unless configuring it otherwise, but in Kotlin static imports like these are more frequent. Meaning that whether they are under Index or Indices is less important.

Why not doing it in a seperate PR? That one is fully backward compatible, which I like.

[rnett]

Another PR is fine, although I don't know that there will be enough impetus to make one, it's a pretty tiny change. I'd like to have infix or .. style creators for most these in Kotlin, anyways.