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.

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

Implement sensitivities for BLAS.gemm #25

Merged
merged 1 commit into from
May 11, 2019
Merged

Implement sensitivities for BLAS.gemm #25

merged 1 commit into from
May 11, 2019

Conversation

ararslan
Copy link
Member

These are ported from Nabla.

willtebbutt
willtebbutt approved these changes Apr 29, 2019
View reviewed changes
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

I've left a comment on the weird failing sensitivity -- I suspect that it's just a minor thing. LGTM other than that, so please do merge once finite-differencing tests pass for alpha.

src/rules/blas.jl Outdated Show resolved Hide resolved
@willtebbutt
Copy link
Member

willtebbutt commented Apr 29, 2019 •
edited
Loading

I've also just realised that the type constraints are probably a bit loose. I think that BLAS routines are usually restricted to strided arrays with particular types of floating-point elements. Perhaps we should do the same here?

@ararslan
Copy link
Member Author

gemm is a case where the methods in LinearAlgebra are, for whatever reason, specified as AbstractMatrix instead of StridedMatrix. That is a good observation about the element type; it's restricted to BlasFloat, and with alpha's type matching the element type of the matrices.

@ararslan ararslan mentioned this pull request May 1, 2019
Closed
@ararslan
Copy link
Member Author

To do:

  • Add tests for non-square matrices

@ararslan
Implement sensitivities for BLAS.gemm
71c018d 
These are ported from Nabla.
@ararslan
Copy link
Member Author

Rectangular tests added. Good to go?

@willtebbutt
Copy link
Member

LGTM

@ararslan ararslan merged commit 45c79e9 into master May 11, 2019
@ararslan ararslan deleted the aa/blas branch May 11, 2019 21:20
@ararslan ararslan mentioned this pull request Jun 18, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willtebbutt willtebbutt willtebbutt approved these changes

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
@ararslan @willtebbutt