-
-
Notifications
You must be signed in to change notification settings - Fork 8
add missing mul! methods with transpose #56
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
Conversation
mcabbott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I think some tests on GPU are in order, as the ones here pass before the PR. (Which is still good, to check that it doesn't break things which worked.)
Also tests for various ways to provide the wrong-sized arrays to this method. (IIRC NNlib.scatter! might be @inbounds.)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
- Coverage 96.26% 95.18% -1.09%
==========================================
Files 3 3
Lines 134 166 +32
==========================================
+ Hits 129 158 +29
- Misses 5 8 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mcabbott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it.
This adds the missing
mul!methods that break the initial (minimal) example in #55. The*method now falls back to this instead of having a separate definition with mutatingNNlib.scatter.It is undoubtedly inefficient to make a separate call to
fill!before executing theNNlibkernel, but I have verified thatNNlibalready does this, so this shouldn't be any different than usingscatter.PR Checklist