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

Generalize or restrict a few basic operators #44564

Merged
merged 6 commits into from
May 13, 2022
Merged

Generalize or restrict a few basic operators #44564

merged 6 commits into from
May 13, 2022

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Mar 11, 2022

This is an attempt to close #43903. While I was at it, I restricted the fallback made the most generic fallbacks simply do what the docstring says, so right and left division operators fall back to multiplication with the inverse.

\(x,y) = adjoint(adjoint(y)/adjoint(x))

to Numbers, and replaced adjoint by conj it by inv(x)*y. The reason is that we don't have a method for adjoint(x::Any), so this fallback is somewhat pointless (edit: unless someone guesses that overloading adjoint is necessary). Also, the docstring of \ doesn't state that it can be defined via / and adjoint. For types that are not subtype of Number or AbstractArray, I believe \ simply needs to be defined explicitly. That's what I have done with ModInt in the generic tests.

@dkarrasch
Copy link
Member Author

One potentially annoying thing with the definition -(x, y) = x + (-1)*y is the following error from CI:

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.

base/operators.jl Outdated Show resolved Hide resolved
@dkarrasch
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

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]>
@StefanKarpinski
Copy link
Sponsor Member

Does this need review to move forward? If so, any idea who?

@dkarrasch
Copy link
Member Author

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.

@stevengj
Copy link
Member

LGTM.

@stevengj stevengj added the needs news A NEWS entry is required for this change label Apr 12, 2022
NEWS.md Outdated Show resolved Hide resolved
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 13, 2022

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
Copy link
Sponsor Member

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

Copy link
Member Author

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.

Copy link
Sponsor Member

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.

@dkarrasch dkarrasch removed the needs news A NEWS entry is required for this change label May 9, 2022
@dkarrasch dkarrasch added the status:merge me PR is reviewed. Merge when all tests are passing label May 9, 2022
@dkarrasch
Copy link
Member Author

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.

@dkarrasch dkarrasch merged commit cf1f717 into master May 13, 2022
@dkarrasch dkarrasch deleted the dk/minus branch May 13, 2022 12:02
@dkarrasch dkarrasch removed the status:merge me PR is reviewed. Merge when all tests are passing label May 13, 2022
@bkamins
Copy link
Member

bkamins commented May 15, 2022

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:

julia> +(nothing)

julia> *(nothing)

julia> +("some string")
"some string"

julia> using Dates

julia> *(Date(2020, 5, 15))
2020-05-15

Thank you!

CC @nalimilan

@dkarrasch
Copy link
Member Author

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 #43903. I might have overdone it with + and *, but I felt that if we have a generic -, then the same should apply to the other basic operations. I'll open a draft PR in which I restrict + and * back to Number, and then we can choose whether we want to revert this one or go forward with the other.

DilumAluthge added a commit that referenced this pull request May 28, 2022
This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl
DilumAluthge added a commit that referenced this pull request Jul 25, 2022
…) (#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]>
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
…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]>
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing fallback methods for vector spaces
7 participants