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

Allow users to specify the output eltype of map #116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MasonProtter
Copy link

The way map currently works is that when you map over an Observable, the eltype of the new Observable is determined by the first application of the mapping function, i.e.

julia> using Observables

julia> o1 = Observable{Any}(true);

julia> o2 = map(o1) do x
           @info "Updating o2 from o1" x
           x + 1
       end
┌ Info: Updating o2 from o1
└   x = true
Observable(2)

julia> eltype(o2) # This is bad!
Int64

julia> o1[] = 1.5 # because of this
┌ Info: Updating o2 from o1
└   x = 1.5
ERROR: InexactError: Int64(2.5)
Stacktrace:
  [1] Int64
    @ ./float.jl:994 [inlined]
  [2] convert(::Type{Int64}, x::Float64)
    @ Base ./number.jl:7
  [3] setproperty!(x::Observable{Int64}, f::Symbol, v::Float64)
    @ Base ./Base.jl:52
[...]

This PR keeps the current default behaviour, but adds a kwarg with two new options out_type = :infer

julia> o2 = map(o1; out_type=:infer) do x
           @info "Updating o2 from o1" x
           x + 1
       end
┌ Info: Updating o2 from o1
└   x = true
Observable{Any}(2)


julia> o1[] = 1.5
┌ Info: Updating o2 from o1
└   x = 1.5
1.5

julia> o2[]
2.5

and out_type::Type:

julia> o3 = map(o2; out_type=Number) do x
           x
       end
Observable{Number}(2.5)

Comment on lines -152 to +159
@test occursin("Observable(5)\n 0 => identity(x) in Base at operators.jl", plain(obs))
@test occursin("Observable(5)\n 0 => identity(x)", plain(obs))
@test string(f) == "ObserverFunction `identity` operating on Observable(5)"
f = on(x->nothing, obs); ln = @__LINE__
str = plain(obs)
@test occursin("Observable(5)", str)
@test occursin("0 => identity(x) in Base at operators.jl", str)
@test occursin(" in Main at $(@__FILE__)", str)
@test occursin("0 => identity(x)", str)
@test occursin(" Main", str)
@test occursin("runtests.jl", str)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to weaken these tests because the printing of this sort of thing has changed on v1.11, and it was causing the tests to fail.

@bvdmitri
Copy link

bvdmitri commented Aug 7, 2024

Perhaps also look at https://github.com/ReactiveBayes/Rocket.jl which already provides this functionality, see here

@SimonDanisch
Copy link
Member

So, I think we specifically had:

o1 = Observable{Any}(true);
 o2 = map!(Observable{Any}(), o1) do x
     @info "Updating o2 from o1" x
     x + 1
 end

For this pattern, which I think is nicer than a keyword argument?

@MasonProtter
Copy link
Author

I find that pattern a lot more annoying to write than using a keyword personally, and second of all, that pattern doesn't cover the :infer keyword I added.

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

Successfully merging this pull request may close these issues.

3 participants