8000 Added the cumsum and cumprod functions similar to numpy's ones. by MarcCote · Pull Request #1700 · Theano/Theano · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added the cumsum and cumprod functions similar to numpy's ones. #1700

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 29, 2014
Merged

Added the cumsum and cumprod functions similar to numpy's ones. #1700

merged 8 commits into from
Jan 29, 2014

Conversation

MarcCote
Copy link
Contributor

This PR adds a cumsum Op which is a wrapper to one from numpy.

Grad is working for any Nd case.

Related to issue #1080
Based on @jsalvatier's gist: https://gist.github.com/jsalvatier/8378901

NEWS.txt:

  • Add theano.tensor.{cumsum,cumprod} as equivalent NumPy function. (Marc-Alexandre Côté, John Salvatier)

@nouiz
Copy link
Member
nouiz commented Jan 28, 2014

In the file theano/tensor/init.py import cumsum to have it available as the numpy interface theano.tensor.cumsum vs numpy.cumsum.

@nouiz
Copy link
Member
nouiz commented Jan 28, 2014

Thanks for this PR! I did some small comment. They are very simple thing and in fact except for the import in tensor/init.py, they are small update that the user won't see, but doing them would be great. Just tell me if you will do them or not.

thanks

@MarcCote
Copy link
Contributor Author
< 8000 div class="edit-comment-hide">

I'm working on it. Also, I was waiting for some feedback before I pushed the cumprod function. Do you want me to include it in this PR, or make another one after the CumsumOp is merged in master?

@nouiz
Copy link
Member
nouiz commented Jan 28, 2014

In the end it is your choise. If the delay is short, it can be simpler for
you to put them in the same PR. But if their is some delays, it can be
useful to merge this one to make it available more rapidly to other users.

On Tue, Jan 28, 2014 at 10:15 AM, Marc-Alexandre Côté <
notifications@github.com> wrote:

I'm working on it. Also, I was waiting for some feedback before I pushed
the cumprod function. Do you want me to include it in this PR, or make
another one after the CumsumOp is merged in master?

Reply to this email directly or view it on GitHubhttps://github.com//pull/1700#issuecomment-33486989
.

@MarcCote
Copy link
Contributor Author

Regarding Theano garbage collector, how can I make sure it is disabled? I printed the address of %(z)s and it always (nil) upon entering the c function. I tried compiling the theano function with linker='cvm_nogc' and also with linker='c' :

        print "A"
        f = theano.function([x], cumsum(x), mode=theano.compile.Mode(linker="cvm_nogc", optimizer="fast_run") )
        print f(a)
        print f(a)

The output:
>>A
>>Ptr of z: (nil)
>>Ptr of z: (nil)

@MarcCote
Copy link
Contributor Author

Ok, I was wrong, I should have used cumsum(x).sum() to have an intermediate result.

@nouiz
Copy link
Member
nouiz commented Jan 28, 2014

Another way to test it is to run with thoses 2 Theano flags:

mode=DebugMode,DebugMode.check_preallocated_output=ALL

This will explicitly test many type of preallocated outputs like if it was
not c contiguous.

On Tue, Jan 28, 2014 at 12:09 PM, Marc-Alexandre Côté <
notifications@github.com> wrote:

Ok, I was wrong, I should have used cumsum(x).sum() to have an
intermediate result.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1700#issuecomment-33499710
.

MarcCote
MarcCote reviewed Jan 28, 2014
View reviewed changes
theano/tensor/tests/test_extra_ops.py
assert np.allclose(np.cumsum(a), f(a)) # Test axis=None

# Test without garbage collector
f = theano.function([x], cumsum(x).sum(), mode=theano.compile.Mode(linker="cvm_nogc", optimizer="fast_run") )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added these flags (mode=DebugMode,DebugMode.check_preallocated_output=ALL) in my .theanorc. Then changed cumsum(x).sum() to cumsum(x). When I run the tests, it still prints (nil) each time the C function is called.

Maybe I didn't understand what @nouiz said.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the theanorc file, they should be like this:

[global]
mode=DebugMode
[DebugMode]
check_preallocated_output=ALL

The syntax isn't the same, but it is related.

On Tue, Jan 28, 2014 at 4:08 PM, Marc-Alexandre Côté <
notifications@github.com> wrote:

In theano/tensor/tests/test_extra_ops.py:

  • def setUp(self):
  •    super(TestCumsumOp, self).setUp()

  •    self.op_class = CumsumOp

  •    self.op = CumsumOp()
  • def test_cumsumOp(self):
  •    x = T.tensor3('x')

  •    a = np.random.random((3, 5, 2)).astype(config.floatX)

  •    b = np.random.random((30, 5, 2)).astype(config.floatX)

  •    f = theano.function([x], cumsum(x), mode="DebugMode")

  •    assert np.allclose(np.cumsum(a), f(a))  # Test axis=None

  •    # Test without garbage collector

  •    f = theano.function([x], cumsum(x).sum(), mode=theano.compile.Mode(linker="cvm_nogc", optimizer="fast_run") )

I've added these flags
(mode=DebugMode,DebugMode.check_preallocated_output=ALL) in my .theanorc.
Then changed cumsum(x).sum() to cumsum(x). When I run the tests, it still
prints (nil) each time the C function is called.

Maybe I didn't understand what @nouiz https://github.com/nouiz said.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1700/files#r9244612
.

@MarcCote
Copy link
Contributor Author

Ok I got it. So I changed the unit tests to use DebugMode(check_preallocated_output="ALL"). If it passes Travis, it would be ready to merge, I guess.

@MarcCote
Copy link
Contributor Author

Hmm don't know what went wrong with Travis.

@nouiz
Copy link
Member
nouiz commented Jan 29, 2014

The travis problem is not related to this PR. It is a temporary problem that is normally fixed by restarting the test job.

I wasn't clear when I talked about check_preallocated_output. We don't want to run all test by default in DebugMode with that parameter, as this make it too long. I just wanted you to run in locally, not update the test to use it. Can you remove your last commit?

This made me see one think that I missed. In test, we normally should use the default mode and not force DebugMode. So can you just remove the mode parameter when you call theano.function()? Otherwise the tests will be too slow by default. Our daily buildbot run all tests with DebugMode.

@MarcCote
Copy link
Contributor Author

Even though it passed the tests with check_preallocated_output, should we not check that strides match in addition to the shape when wrapping the xdecref/allocation? I asked that because I'm implementing a personal Op and it was causing memory error when omitting the strides check.

EDIT: It might have something to do with my personnal Op printing warning message about Stride mismatch!

@nouiz
Copy link
Member
nouiz commented Jan 29, 2014

This depend if the code depend of the strides. I suppose in your personnal
op you where supposing the output was c contigious.

In this case, PyArray_CumSum() don't do that supposition and check the
strides of the outputs. So you don't need check the strides for this op.

On Wed, Jan 29, 2014 at 1:16 PM, Marc-Alexandre Côté <
notifications@github.com> wrote:

Even though it passed the tests with check_preallocated_output, should we
not check that strides match in addition to the shape when wrapping the
xdecref/allocation? I asked that because I'm implementing a personal Op and
it was causing memory error when omitting the strides check.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1700#issuecomment-33612993
.

< A7EE /p>

@nouiz
Copy link
Member
nouiz commented Jan 29, 2014

Thanks, I merge it. @jsalvatier thanks for the first version. It is now merged in Theano.

nouiz added a commit that referenced this pull request Jan 29, 2014
@nouiz
Merge pull request #1700 from MarcCote/cumsum
262f59a 
Added the cumsum and cumprod functions similar to numpy's ones.
@nouiz nouiz merged commit 262f59a into Theano:master Jan 29, 2014
@MarcCote MarcCote deleted the cumsum branch January 29, 2014 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MarcCote @nouiz
0