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

Define +(::UniformScaling{Bool}) #36668

Closed
ronisbr opened this issue Jul 14, 2020 · 4 comments · Fixed by #36784
Closed

Define +(::UniformScaling{Bool}) #36668

ronisbr opened this issue Jul 14, 2020 · 4 comments · Fixed by #36784
Labels
linear algebra Linear algebra

Comments

@ronisbr
Copy link
Sponsor Member

ronisbr commented Jul 14, 2020

In Julia, we can use -I:

julia> -I
UniformScaling{Int64}
-1*I

which is exactly the same as -1*I. However, +I is not defined:

julia> +I
ERROR: MethodError: no method matching +(::UniformScaling{Bool})
Closest candidates are:
  +(::UniformScaling, ::Number) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.4/LinearAlgebra/src/uniformscaling.jl:109
  +(::UniformScaling, ::UniformScaling) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.4/LinearAlgebra/src/uniformscaling.jl:114
  +(::UniformScaling, ::BitArray{2}) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.4/LinearAlgebra/src/uniformscaling.jl:116
  ...
Stacktrace:
 [1] top-level scope at REPL[136]:1

This seems confusing at first glance. Hence, I propose that we define +I as +1*I to keep consistency.

If this is approved, then I can do a PR.

@dkarrasch
Copy link
Member

Why not have

(+)(J::UniformScaling) = J

to avoid unnecessary promotion of J.λ?

@dkarrasch dkarrasch added the linear algebra Linear algebra label Jul 15, 2020
@mbauman
Copy link
Sponsor Member

mbauman commented Jul 15, 2020

I think I'd do:

(+)(J::UniformScaling) = UniformScaling(+J.λ)

@ronisbr
Copy link
Sponsor Member Author

ronisbr commented Jul 15, 2020

Why not have

(+)(J::UniformScaling) = J

to avoid unnecessary promotion of J.λ?

The only problem here is that we will have a different behavior between - and +:

julia> import Base: +

julia> (+)(J::UniformScaling) = J
+ (generic function with 290 methods)

julia> -I
UniformScaling{Int64}
-1*I

julia> +I
UniformScaling{Bool}
true*I

I think I'd do:

(+)(J::UniformScaling) = UniformScaling(+J.λ)

This seems to be fine since it replicates that same behavior of -:

julia> import Base: +

julia> (+)(J::UniformScaling) = UniformScaling(+J.λ)
+ (generic function with 167 methods)

julia> -I
UniformScaling{Int64}
-1*I

julia> +I
UniformScaling{Int64}
1*I

I will wait a couple of days for any new ideas and then I submit the PR.

@ronisbr
Copy link
Sponsor Member Author

ronisbr commented Jul 23, 2020

Done! In Brazil, couple of days === 8 days 😅 (sorry for the delay, completely forgot about this...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants