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

Add dot(x,A,y) #683

Merged
merged 12 commits into from
Dec 27, 2019
Merged

Add dot(x,A,y) #683

merged 12 commits into from
Dec 27, 2019

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Dec 4, 2019

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?

README.md Outdated Show resolved Hide resolved
Co-Authored-By: Fredrik Ekre <[email protected]>
@martinholters
Copy link
Member

Removing the deprecations makes this a breaking change, which I'd like to avoid. If you use dot qualified everywhere (i.e. LinearAlgebra.dot), that should go a long way. What other problems appear with the deprecations in place?

@martinholters
Copy link
Member

Ok, I've re-enabled the deprecations after some tweaking.

@martinholters
Copy link
Member

Can we get some nicer code formatting here? E.g. no lines longer than 92 chars and import and using statements collected at the top. (Yes, I missed the opportunity to do so in the commit I've pushed.)

README.md Outdated Show resolved Hide resolved
mcabbott and others added 2 commits December 26, 2019 04:14
Co-Authored-By: Martin Holters <[email protected]>
@mcabbott
Copy link
Contributor Author

See if you like the current version, not strictly 92 but narrower than earlier things. And thanks for fixing the deprecations earlier!

Copy link
Member

@martinholters martinholters left a 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
Copy link
Member

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.

Copy link
Contributor Author

@mcabbott mcabbott Dec 26, 2019

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.

src/Compat.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Contributor Author

The generic case is tested, in that commenting it out leads to MethodError: no method matching dot(::Int64, ::Int64, ::Int64). I can't see a test for Diagonal in https://github.com/JuliaLang/julia/pull/32739/files , so I've invented one.

@martinholters
Copy link
Member

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.

@martinholters martinholters merged commit 601e535 into JuliaLang:master Dec 27, 2019
@mcabbott mcabbott deleted the dot branch December 31, 2019 11:27
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.

3 participants