-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
Add dot(x,A,y) #683
Add dot(x,A,y) #683
Conversation
Co-Authored-By: Fredrik Ekre <[email protected]>
Removing the deprecations makes this a breaking change, which I'd like to avoid. If you use |
Ok, I've re-enabled the deprecations after some tweaking. |
Can we get some nicer code formatting here? E.g. no lines longer than 92 chars and |
Co-Authored-By: Martin Holters <[email protected]>
See if you like the current version, not strictly 92 but narrower than earlier things. And thanks for fixing the deprecations earlier! |
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.
Generally looks good to me modulo some minor remarks.
Also, I'm not sure all methods are tested; especially I couldn't find tests for the generic fallback and the ::Diagonal
one. (I may easily have missed them, though.)
require_one_based_indexing(A...) = !Base.has_offset_axes(A...) || throw(ArgumentError( | ||
"offset arrays are not supported but got an array with index other than 1")) | ||
else | ||
using Base: require_one_based_indexing |
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.
I'm not sure we should be using this. It's an internal function that might just go away eventually. Unless I missed something, it's used only once above, so we could just unconditionally replace it with its definition there. Or at least, we should move this block into the other conditional, so that Base.require_one_based_indexing
is only resolved for Julia versions where we know it exists.
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.
Indeed it's only used once now, although I think I have run into it elsewhere. The reason to add it was just to keep the dot
methods as close to identically copied as possible, rather than inlining the definition.
But indeed if say Julia 1.9 renames this thing, Compat shouldn't break. I've now added a bound for the Base function too.
The generic case is tested, in that commenting it out leads to |
I've added a merge of master and moved the README entry to the top of the list, and bumped the version to 3.2.0. Once CI is green, I'll merge and tag a release. |
This adds most of JuliaLang/julia#32739.
I got tired of copying and skipped some of the more exotic matrix types. They should all hit the generic fallback though, so should still work.
I commented out bits of
src/deprecated.jl
to make tests pass. I’m not too sure what those are meant to do, and whether they are still needed post-1.0?