Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Added support for 'logSumExp'. #150

Merged
merged 15 commits into from
Jun 22, 2019
Merged

Conversation

eaplatanios
Copy link
Contributor

Depends on #149. @rxwei I was also relying on a general Tensor.withoutDerivative() for this. It'd be nice to find a nice way to deal with this situation more generally.

@eaplatanios eaplatanios requested a review from rxwei May 31, 2019 01:21
@eaplatanios eaplatanios added the enhancement New feature or request label May 31, 2019
@rxwei
Copy link
Contributor

rxwei commented Jun 21, 2019

Could you use the global Swift.withoutDerivative(at:in:) for now?

@eaplatanios eaplatanios changed the title Added support for 'logSumExp'. [WIP] Added support for 'logSumExp'. Jun 21, 2019
@eaplatanios
Copy link
Contributor Author

I addressed that. It remains to add documentation and tests which I'll do later today.

@eaplatanios eaplatanios changed the title [WIP] Added support for 'logSumExp'. Added support for 'logSumExp'. Jun 22, 2019
@eaplatanios
Copy link
Contributor Author

@rxwei I added documentation and tests. The CI tests are running now and so this only requires an approving review.

@eaplatanios
Copy link
Contributor Author

CI tests pass so this is only pending a review.

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!

@rxwei
Copy link
Contributor

rxwei commented Jun 22, 2019

@rxwei I was also relying on a general Tensor.withoutDerivative() for this. It'd be nice to find a nice way to deal with this situation more generally.

Yeah I saw your use case. I think this justifies having a top-level withoutDerivative<T>(_ x: T) -> T identity function.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request kokoro:run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants