-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Generalize or restrict a few basic operators #44564
Conversation
One potentially annoying thing with the definition Test Failed at /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/errorshow.jl:401
Expression: occursin("MethodError: no method matching -(::Vector{Float64}, ::Rational{$(Int)})", err_str)
Evaluated: occursin("MethodError: no method matching -(::Vector{Float64}, ::Rational{Int64})", "MethodError: no method matching +(::Vector{Float64}, ::Rational{Int64})\nFor element-wise addition, use broadcasting with dot syntax: array .+ scalar\nClosest candidates are:\n +(::Any, ::Any, !Matched::Any, !Matched::Any...) at operators.jl:593\n +(!Matched::Base.TwicePrecision, ::Number) at twiceprecision.jl:290\n +(!Matched::LinearAlgebra.UniformScaling, ::Number) at ~/buildbot/worker-tabularasa/tester_macos64/build/share/julia/stdlib/v1.9/LinearAlgebra/src/uniformscaling.jl:144\n ...") So the error message becomes slightly misleading. |
@nanosoldier |
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
Update base/operators.jl Co-authored-by: Martin Holters <[email protected]>
Does this need review to move forward? If so, any idea who? |
Yes, I think so. Would be good if some of the core devs could take a look if these are reasonable as fallbacks. Maybe @stevengj, @vtjnash, @JeffBezanson for a start? The tests and the ecosystem seem happy, but there may be underlying assumptions that are not laid out in tests. |
LGTM. |
I don't know the math sufficiently well to claim whether these are equivalent |
+(x::Number) = x | ||
*(x::Number) = x | ||
+(x) = x | ||
-(x) = Int8(-1)*x |
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.
this seems possibly incorrect for many types (e.g. Unsigned), and less likely for promotion to be defined than negation
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.
This was the main requested feature in the related issue. This change causes a different error than the one previously expected in the int.jl
tests.
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.
gotcha. As I replied above, I have no context for this change, so I am mostly the wrong person to ask.
Co-authored-by: Jameson Nash <[email protected]>
Are the ambiguity test failures in linuxaarch64 real? If not (tests pass on all other platforms or fail for obviously unrelated reasons), then this can be merged. |
This PR breaks DataFrames.jl tests (fortunately tests only). Before I start fixing DataFrames.jl could you please explain me the rationale behind allowing for example:
Thank you! CC @nalimilan |
Is the failing test a new one? DataFrames.jl was not listed as failing in the nanosoldier run from March. I'm very sorry for the breakage! The general idea of this PR is to make vector space calculus the generic default as discussed in JuliaLang/LinearAlgebra.jl#905. I might have overdone it with |
This reverts commit cf1f717. Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl
…) (#45489) * Revert "Remove type-unlimited unary `+` and `*` (#45320)" This reverts commit 990b1f3. * Revert "Generalize or restrict a few basic operators (#44564)" This reverts commit cf1f717. Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl Co-authored-by: Daniel Karrasch <[email protected]>
…and JuliaLang#45320) (JuliaLang#45489) * Revert "Remove type-unlimited unary `+` and `*` (JuliaLang#45320)" This reverts commit 990b1f3. * Revert "Generalize or restrict a few basic operators (JuliaLang#44564)" This reverts commit cf1f717. Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl Co-authored-by: Daniel Karrasch <[email protected]>
…and JuliaLang#45320) (JuliaLang#45489) * Revert "Remove type-unlimited unary `+` and `*` (JuliaLang#45320)" This reverts commit 990b1f3. * Revert "Generalize or restrict a few basic operators (JuliaLang#44564)" This reverts commit cf1f717. Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl Co-authored-by: Daniel Karrasch <[email protected]>
This is an attempt to close JuliaLang/LinearAlgebra.jl#905. While I was at it, I
restricted the fallbackmade the most generic fallbacks simply do what the docstring says, so right and left division operators fall back to multiplication with the inverse.toit byNumber
s, and replacedadjoint
byconj
inv(x)*y
. The reason is that we don't have a method foradjoint(x::Any)
, so this fallback is somewhat pointless (edit: unless someone guesses that overloadingadjoint
is necessary). Also, the docstring of\
doesn't state that it can be defined via/
andadjoint
. For types that are not subtype ofNumber
orAbstractArray
, I believe\
simply needs to be defined explicitly. That's what I have done withModInt
in thegeneric
tests.