Closed hhaensel closed 3 years ago
The concern here is that this hurts inferrability by changing the struct to have an abstract field. The MWE you linked, https://github.com/GenieFramework/Stipple.jl/issues/56, is about a Reactive which AFAIK is not an AbstractObservable (and Reactive.jl seems effectively unmaintained, so we should all switch to Observables). I therefore don't understand how this fixes that problem, and since there would be a significant downside to merging this, can you explain a bit more? Maybe with a test?
First of all, the Reactive
is misleading, it is not referring to the Package Reactive.jl
but to the main type of the Package Stipple.jl
which is a subtype of AbstractObservable
.
mutable struct Reactive{T} <: Observables.AbstractObservable{T}
o::Observables.Observable{T}
[...]
end
The irritating point for me is that the function map!()
is defined for AbstractObservables
whereas the call of MapUpdater only accepts only Observable
.
@inline function Base.map!(f::F, observable::AbstractObservable, os...; update::Bool=true) where F
# note: the @inline prevents de-specialization due to the splatting
obsfuncs = onany(MapUpdater(f, observable), os...)
appendinputs!(observable, obsfuncs)
if update
observable[] = f(map(to_value, os)...)
end
return observable
end
struct MapUpdater{F, T} <: Function
f::F
observable::Observable{T}
observable::AbstractObservable{T}
end
You could probably as well define map!()
to accept only Observable{T}
.
I'm not sure whether I understand the type inference problem completely and whether this problem would occur in our use case.
julia> x = Reactive(10)
Reactive{Int64}(Observable{Int64} with 0 listeners. Value:
10, 0, false, false)
julia> map!(sqrt, x, 16)
Observable{Int64} with 0 listeners. Value:
4
julia> x
Reactive{Int64}(Observable{Int64} with 0 listeners. Value:
4, 0, false, false)
Having said all this I must say that the pain for our Reactive type is not very big, as we can always refer to the Obersable field.
julia> map!(sqrt, x.o, 16)
Observable{Int64} with 0 listeners. Value:
4
I'm not sure whether I understand the type inference problem completely
struct MyType x::AbstractFloat end f(mt::MyType) = mt.x
y = MyType(1.0)
followed by
![image](https://user-images.githubusercontent.com/1525481/136695323-f3c4f1ca-b8de-43d5-9ba5-3f15135db994.png)
should hopefully help explain. If you need this to work, given that it's better not to break inferrability I think you probably should add another type-parameter to `MapObservable`.
I understand better now, thanks. I'm not how I should add another type-parameter, though. But wouldn't the following code also solve the issue?
import Base.map!
@inline Base.map!(f::F, r::Reactive, os...; update::Bool=true) where F = Base.map!(f, r.o, os...; update=update)
At least I receive the following result:
julia> r1 = Reactive(1.0)
Reactive{Float64}(Observable{Float64} with 0 listeners. Value:
1.0, 0, false, false)
julia> r2 = Reactive(1.0)
Reactive{Float64}(Observable{Float64} with 0 listeners. Value:
1.0, 0, false, false)
julia> map!(sqrt, r2, r1)
Observable{Float64} with 0 listeners. Value:
1.0
julia> r1[] = 16
16
julia> r2
Reactive{Float64}(Observable{Float64} with 0 listeners. Value:
4.0, 0, false, false)
I'm not how I should add another type-parameter, though.
struct MapUpdater{F, T, O<:Observable{T}} <: Function
f::F
observable::O
end
But wouldn't the following code also solve the issue?
Fine with me. If you're happy with that, that can go in the package that defines Reactive
and you can close this and the issue.
If you're not happy with that, feel free to add the version with the extra type parameter.
I think I'm fine with my proposal and we can leave Observables as is. Thanks for your advise; that gave me the thought in the right direction 😃
This is the PR to #72
Currently, Observables throws an error when trying to
map!(any, r, o)
where o is a normal Observable and r is of a different observable typeR{T} <: AbstractObservable{T}
.As
map!()
,map()
andconnect!()
all supportAbstractObservables
, this is probably the only place, that needs to be adapted.