Optimize to_rarray to avoid recursion for simple inputs
#140
Conversation
|
|
||
| to_rarray(x::Number) = x # TODO: should this be a `ConcreteRArray{_,0}`? | ||
| to_rarray(x::ConcreteRArray) = x | ||
| to_rarray(x::AbstractArray{<:Number}) = ConcreteRArray(x) |
There was a problem hiding this comment.
cc @avik-pal can we make a union type of all the primitive types that Reactant supports conversion into (e.g. int, float, complex float, etc). We should then use this instead of Number here
There was a problem hiding this comment.
union over https://mlir.llvm.org/docs/Dialects/Builtin/#types? (atleast the ones that are in base)
There was a problem hiding this comment.
@gdalle I think we're just waiting for this to get addressed, then lgtm
There was a problem hiding this comment.
It's merged but we still shouldn't do to_rarray(x::Number) = x because the semantics won't work out
|
Now that Adapt is a strong dependency, shouldn't we just overload |
Well to_rarray predates adapt as a strong dep [which we might remove if we can work around later], but also are we sure that adapt correctly deals with recursive data structures? |
Not really. You need to opt-in for every custom structure |
But then, we could maybe overload |
Yeah, but we introduced |
|
What should we do about this PR? |
I guess we can talk about this on Wednesday. Would you mind putting it in the agenda? |
Fixes #138 by adding methods for
to_rarrayon::Number,::AbstractArray{<:Number}and::ConcreteRArray. Adds tests with JET to ensure type stability (which did not hold before).Question: what should happen to a number going through
to_rarray? If it keeps the same type, how can it be traced through the function?