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

adjoints for cumsum and cumprod #294

Closed

Conversation

kraftpunk97-zz
Copy link

@kraftpunk97-zz kraftpunk97-zz commented Aug 13, 2019

Another attempt at #282 .

cumsum is just reverse(cumsum(reverse(∆, dims=dims), dims=dims), dims=dims), as suggested by @mcabbott .

With cumprod, I had help from @dhairyagandhi96 who asked me to refer to PyTorch's implementation

I've had no way to test my gradients except for comparing my results with that of torch.cumprod for same inputs, so I haven't added any tests yet

@kraftpunk97-zz
Copy link
Author

kraftpunk97-zz commented Aug 13, 2019

Another thing that comes to mind... I've had trouble getting the adjoints to work if I define them directly for the functions. However, if I wrap them around another function (like my_cumprod or my_cumsum) and then define their adjoints and call the gradient on these wrapper functions, they work.

The error I get is a DimensionMismatchError. I have copied the stack trace I get for cumprod.

@DhairyaLGandhi
Copy link
Member

Something seems to have messed up in this commit?

@kraftpunk97-zz
Copy link
Author

It's probably nothing. When I open the file in Github's editor, the formatting seems fine.

@DhairyaLGandhi
Copy link
Member

Good to look into why, probably just editor settings.

@kraftpunk97-zz
Copy link
Author

kraftpunk97-zz commented Sep 1, 2019

Unable to identify issue. Although I did find the same behaviour here, if relevant.

@CarloLucibello
Copy link
Member

cumsum has already been implemented. We can merge the cumprod part of this PR if you rebase and fix the formatting issues

@CarloLucibello
Copy link
Member

this is no longer needed

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.

4 participants