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

Trait-based dispatch of number methods #155

Closed
shashi opened this issue Jan 5, 2021 · 8 comments
Closed

Trait-based dispatch of number methods #155

shashi opened this issue Jan 5, 2021 · 8 comments

Comments

@shashi
Copy link
Member

shashi commented Jan 5, 2021

Parameter{X<:Number} type from ModelingToolkit is used as a parameter as in Symbolic{Parameter{X}}, but that means methods in src/methods.jl cannot be restricted to Symbolic{<:Number} because it won't work on MTK parameters.

There are 2 approaches to fixing this:

  • Make Parameter it's own type which is a subtype of Symbolic{<:Number} -- not yet sure how the whole system will work when adding a new type. See here for an example of this kind of circus
  • Make @number_methods work based on dispatch and disregard Symbolic{T}, or optionally just eliminate the type parameter and useSymbolic.
@ChrisRackauckas
Copy link
Member

I would propose using @MasonProtter's idea from

JuliaLang/julia#37790 (comment)

of how to do type tags in the current type system. Sym{ModelingToolkit.Parameter{Real}} is a non-extendable version of this, and interferes with the current rules handling. If it's always Sym{Real,{ModelingToolkit.Parameter}}, then the contextual information is all in extendable tags and the rules can be written to use the algebraic set of the first part.

@MasonProtter
Copy link
Member

MasonProtter commented Jan 5, 2021

The inevitable problem is how do we make sense of many tags? I really like the idea of these type tags, but then I think about

Sym{Real, Tuple{Parameter, Tag2, Tag3, Tag4}}

and I realize that anyone who handles this thing is going to need to know not just what Parameter is, but also what Tag2, Tag3, and Tag4 are.

@MasonProtter
Copy link
Member

I guess if we have a rule that "tags must be ignorable" we could probably be okay and then use Union instead of Tuple. This means that if someone is writing a method, they should always be free to not take advantage of tags and still have a correct program.

That means that to go back to the motivating example in the other thread, MyArray{T, N, Tags{Adjoint}} would be bad because the presence of the Adjoint tag changes how you should interpret the data, but MyArray{T, N, Tags{Sorted}} would be fine because the Sorted is just going to give optimization opportunities.

@ChrisRackauckas
Copy link
Member

I think the rule for the symbolic case is "tags must be ignorable". For something like Parameter, just the MTK equation parsing code needs to know what a Parameter is, so it would work great here. We can fit all other context in there, and the simplification algorithm should feel free to ignore it all. The system construction will look for it though in order to know the context of the variables.

@shashi
Copy link
Member Author

shashi commented Jan 6, 2021

Union sounds interesting. But how do you dispatch on just Parameter when you have a Union tag?

@MasonProtter
Copy link
Member

Here's a MWE:

struct Tag1 end
struct Tag2 end
struct Tag3 end
struct Parameter end
struct Sym{T, U}
    name::Symbol
end

f(x::Sym{Real, Union{Parameter, Other}}) where {Other} = "hi"
f(x::Sym{Real, Union{Other}}) where {Other} = "bye"

x = Sym{Real, Union{Tag1, Tag2, Parameter, Tag3}}(:x)
y = Sym{Real, Union{Tag1, Tag2, Tag3}}(:y)
julia> f(x)
"hi"

julia> f(y)
"bye"

@MasonProtter
Copy link
Member

Note however, there's trouble if you put a UnionAll type in the Union: JuliaLang/julia#37793. Jameson claims he intends to make this not work at all, which I think is horrible, but it's something to be aware of.

@shashi
Copy link
Member Author

shashi commented Apr 1, 2021

The original intention of this issue has been addressed by #259

@YingboMa wants to use traits to better handle things like quaternion multiplication. Let's have a fresh issue for that.

@shashi shashi closed this as completed Apr 1, 2021
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