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

Use of promote_op problematic #396

Open
baggepinnen opened this issue Nov 19, 2020 · 3 comments
Open

Use of promote_op problematic #396

baggepinnen opened this issue Nov 19, 2020 · 3 comments

Comments

@baggepinnen
Copy link
Member

See discussion in JuliaLang/julia#38436 (comment)

We should try to rid ourselves of the use of promote_op to avoid future situations like this.

@mfalt
Copy link
Member

mfalt commented Nov 20, 2020

I agree that we should fix this. It should be fairly straight forward if we use one(T) and zero(T) to compute the promotion type instead. However, the following in promotion.jl will be trickier

function Base.promote_rule(::Type{StateSpace{TE, T1, MT}}, ::Type{T2}) where {TE, T1, MT, T2<:Number}
    NewMT = Base.promote_op(*, MT, T2)
    StateSpace{TE, eltype(NewMT), NewMT}
end

@olof3
Copy link
Contributor

olof3 commented Nov 20, 2020

Didn't we say that we should get rid of the Matrix type argument? Which would resolve this?

@mfalt
Copy link
Member

mfalt commented Nov 20, 2020

I assume we would have a similar problem for the corresponding HeteroStateSpace, but we seem to avoid defining promote rules with numbers by calling the constructors in those cases, so MAYBE that is the better workaround.
But we seem to be inconsistent/wrong with the current implementations, e.g in types/promotion.jl

*(sys::ST, n::Number) where ST <: AbstractStateSpace = StateSpace(sys.A, sys.B, sys.C*n, sys.D*n, sys.timeevol)
/(sys::ST, n::Number) where ST <: AbstractStateSpace = ST(sys.A, sys.B, sys.C/n, sys.D/n, sys.timeevol)

and I can't see directly if we catch the matrix case anywhere.

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

No branches or pull requests

3 participants