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 triangle/nonzero functions #1181

Merged
merged 17 commits into from Feb 18, 2013
Merged

Add triangle/nonzero functions #1181

merged 17 commits into from Feb 18, 2013

Conversation

jlowin
Copy link
Contributor

@jlowin jlowin commented Jan 29, 2013

Triangle:

  • Adds Tri op
  • Adds tensor.tri, tensor.triu, and tensor.tril functions that wrap Numpy equivalents

Nonzero:

  • Adds Nonzero op
  • Adds tensor.nonzero, tensor.flatnonzero functions that wrap Numpy equivalents
  • Adds tensor.nonzero_values to get around lack of advanced indexing for nonzero elements
  • Unit tests

@jlowin
Copy link
Contributor Author

jlowin commented Jan 29, 2013

Couple notes -- numpy.nonzero() always returns a tuple of indices, one for each dimension. I could only get the Nonzero Op to return a list, not a tuple. Also, it won't return a list of length 1, returning instead the sole Apply instance. So for ndim > 1, numpy and this Op both return sequences of indices, but for ndim == 1, numpy returns a length-1 tuple and this op returns a Nonzero object. If anyone can tell me how to address either issue, of course I'll change that.

@jlowin
Copy link
Contributor Author

jlowin commented Jan 29, 2013

This behavior for extracting all nonzero elements is supported by Numpy but not by Theano:

a[np.nonzero(a)]

Even doing the indexing explicitly in Theano only works for matrices:

a[tensor.nonzero(a)[0], tensor.nonzero(a)[1]])

So I've added a helper function, tensor.nonzero_values, to get around the advanced indexing problem. Calling tensor.nonzero_values(a) or a.nonzero_values() in Theano is equivalent to calling a[np.nonzero(a)] in Numpy. This relies on another numpy function, flatnonzeros, which I've also added to Theano.

@nouiz
Copy link
Member

nouiz commented Jan 29, 2013

I'll let @lamblin review this, but there is another PR that implement the full advanced indexing. So don't loose too much time working around that!

gh-1083

@jlowin
Copy link
Contributor Author

jlowin commented Jan 29, 2013

I know! I'm eagerly waiting for it. But don't worry, I didn't waste any time -- it probably took longer to write the docstring than the function. It's basically just a convenience wrapper around

a.flatten()[tensor.nonzero(a.flatten())]

@lamblin
Copy link
Member

lamblin commented Jan 29, 2013

Rather than output a list of ndim different outputs for a NonZero Op, which all have the same length, I think it would be better (more Theano-like) to have only one matrix output, of shape (ndim, nnonzero).
The reason numpy puts the output in a tuple of vectors is to enable the indexing syntax, but since it is not supported by Theano, I don't think it is necessary.
At least, I think that the Op should have only one matrix output, but then, if needed, a tensor.nonzero function could return something like [_nonzero(a)[i] for i in range(ndim)].

@jlowin
Copy link
Contributor Author

jlowin commented Jan 29, 2013

That makes sense, and I agree having the Op output a matrix is more Theano-like.

I hesitate a little bit on the tensor.nonzero function returning a matrix because I know a lot of effort goes into keeping things as similar to Numpy as possible. But, as you say, Theano can't even use the output in the same way (hence, nonzero_values) so I'm not sure if that rule of thumb should apply.

I'll change the Op now, and let me know what you prefer for the function and I'll make the change as well.

@nouiz
Copy link
Member

nouiz commented Jan 30, 2013

travis-ci have a few errors. Can you check them?

@jlowin
Copy link
Contributor Author

jlowin commented Jan 30, 2013

My local tests are passing, I'll see if it's something happening with 2.7.3 vs 2.5.

Edit -- no, looks like older versions of numpy complain about concatenating zero-length arrays. submitting something to address that and the ensuing casting issue.

@jlowin
Copy link
Contributor Author

jlowin commented Jan 31, 2013

Hopefully these changes will address the issues with older versions of numpy -- how do I get travis to run?

@lamblin
Copy link
Member

lamblin commented Jan 31, 2013

The latest version of the PR seems to be in Travis's queue: https://travis-ci.org/Theano/Theano/pull_requests

@jlowin
Copy link
Contributor Author

jlowin commented Feb 1, 2013

It seems to have had an error before starting

lamblin added a commit that referenced this pull request Feb 18, 2013
Add triangle/nonzero functions
@lamblin lamblin merged commit d7e822c into Theano:master Feb 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants