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 Signals to be used as view indices #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow Signals to be used as view indices #124

wants to merge 2 commits into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 6, 2017

This one is one of those "boy, julia sure is fun" changes. The motivation for this was to support a more flexible approach to "movie player" GUIs. I've written 3 separate player GUIs over my career, and I've never been happy with how important information gets "buried" inside the visualization code (which makes it hard to extend). For example:

  • what if you have two movies, and you want the play button/slider to keep the two movies in sync?
  • what if the user wants to compute (at the REPL prompt) the image's mean intensity over the current zoom region?

All this involves getting information out of the GUI. What I finally realized is that perhaps the better strategy is to inject the necessary information into the GUI...or rather, the data itself. A small demo:

julia> using Reactive

julia> A = reshape(1:3*4*2, 3, 4, 2)
3×4×2 Base.ReshapedArray{Int64,3,UnitRange{Int64},Tuple{}}:
[:, :, 1] =
 1  4  7  10
 2  5  8  11
 3  6  9  12

[:, :, 2] =
 13  16  19  22
 14  17  20  23
 15  18  21  24

julia> s = CheckedSignal(1, 1:2)
Reactive.CheckedSignal{Int64,UnitRange{Int64}}(Signal{Int64}(1, nactions=0),1:2)

julia> V = view(A, :, :, s)
3×4 SubArray{Int64,2,Base.ReshapedArray{Int64,3,UnitRange{Int64},Tuple{}},Tuple{Colon,Colon,Reactive.CheckedSignal{Int64,UnitRange{Int64}}},false}:
 1  4  7  10
 2  5  8  11
 3  6  9  12

julia> push!(s, 2)

julia> V
3×4 SubArray{Int64,2,Base.ReshapedArray{Int64,3,UnitRange{Int64},Tuple{}},Tuple{Colon,Colon,Reactive.CheckedSignal{Int64,UnitRange{Int64}}},false}:
 13  16  19  22
 14  17  20  23
 15  18  21  24

With this, you implement a movie player as a dumb 2d image viewer, and provide a controller GUI whose only role is to modifiy s. This keeps the two aspects of the GUI orthogonal, and so it's easy to do more complicated stuff. For example, if you use the same s in two arrays, you keep two movies in sync; if you use

indsy, indsx = indices(img, 1), indices(img, 2)
zoomy, zoomx = CheckedSignal(indsy, indsy), CheckedSignal(indsx, indsx)
imgz = view(img, zoomy, zoomx)

then you can "access" the same data being shown in the viewer through imgz.

What's also nice is that the performance isn't terrible:

julia> function mysum(A)
           accum = zero(eltype(A))
           for i in CartesianRange(indices(A))
               accum += A[i]
           end
           accum
       end
mysum (generic function with 1 method)

julia> A = rand(1000,1000,3);

julia> s
Reactive.CheckedSignal{Int64,UnitRange{Int64}}(Signal{Int64}(2, nactions=0),1:2)

julia> V = view(A, :, :, s);

julia> V1 = freeze(V);  # constructs a "regular" view

julia> mysum(V)
499977.50044777873

julia> mysum(V1)
499977.50044777873

julia> @time mysum(V)
  0.003886 seconds (5 allocations: 176 bytes)
499977.50044777873

julia> @time mysum(V1)
  0.002645 seconds (5 allocations: 176 bytes)
499977.50044777873

In terms of implementation, I'm aware of the fact that you could make a value-checked signal via map on an existing signal or two. But for the purposes of the array code I really wanted to be able to use the type system to ensure that bounds-checking was activated. So in the end I decided to create a new type. If you object to the complexity (I know @shashi worked hard to slim down the number of types), we should discuss whether there are good alternatives.

It's also nice that it only took 13 lines of code to pull off the necessary array changes. Julia is fun. 😄

CC @Cody-G, @SimonDanisch, @mbauman.

@mbauman
Copy link

mbauman commented Mar 6, 2017

I'm afraid my recent work on 0.6 is going to stymie you here; view on 0.6 calls to_indices before constructing the subarray, effectively freezing the signal before any of the fun begins. You should still be able to hack around that by specializing view with Signal indices to prevent that from occurring.

@timholy
Copy link
Member Author

timholy commented Mar 6, 2017

Yeah, I figured I'd have to change stuff for 0.6. Even though I haven't really looked, I'm not too worried.

@shashi
Copy link
Member

shashi commented Mar 7, 2017

This is pretty cool! Those are some nice examples. I like that this makes core.jl internals more generic so that packages can create their own types like this.

But for the purposes of the array code I really wanted to be able to use the type system to ensure that bounds-checking was activated.

I don't think I understand this. Can you explain this a little bit?

So is it the case that the video player keeps reading the SubArray object repeatedly and drawing that to the screen? I'm wondering what notifies it.

I'm slightly concerned about the dependency on OffsetArrays. I suspect that will be hard to drop. I'm fine with keeping it if so.

@timholy
Copy link
Member Author

timholy commented Mar 7, 2017

But for the purposes of the array code I really wanted to be able to use the type system to ensure that bounds-checking was activated.

I don't think I understand this. Can you explain this a little bit?

Sure (sorry, it's a bit long). If you're going to use a Signal as an index to an array, at some point you're going to have to check bounds. There are three times where you might do this:

  1. when you first create the SubArray
  2. whenever you access values of the SubArray
  3. whenever you push! values to the Signal

For Signals, 1 is essentially useless---yes you should do it then, but it's not remotely adequate since the value of the signal might change. So the interesting cases are 2 and 3. One important thing to keep in mind is that if I create a V = view(A, :, 3, :) from a 3d array A, V only takes 2 indices (the slice gets rid of the second dimension of A). So in ordinary usage, we don't have to keep checking bounds on the 3, because that was checked upon construction, and it's only "user indices" i, j in V[i, j] that must be checked.

With the Signals version V = view(A, :, s, :), you would want to check the value of s each time (this is option 2). That could be done most safely between these two lines, but for performance reasons you'd like to put a @boundscheck in front of that (so that inside of @inbounds that check gets skipped). That would mostly be safe, even for code like copy!(dest, V) where

function copy!(dest, src)
    indices(dest) == indices(src) || error("oops")
    @inbounds for i in eachindex(src)
        ...

The reason this would be safe is that the initial indices(src) call would fail due to the boundscheck on the signal. If copy! yielded during the inner loop it would no longer be safe (a user might change the value of the signal halfway through the copy), but if we close our eyes and clap our heels together we can pretend that we can ignore that problem.

Unfortunately, option 2 doesn't have an adequate solution for this:

function mysum(A)
    accum = zero(eltype(A))
    @inbounds for a in A
        accum += a
    end
    accum
end

This would not check bounds because the creation of the iterator (which calls indices which should provide safety) is inside an @inbounds. So you get a segfault if the user has pushed an invalid value for the signal, even though such code would never segfault for a "static" view.

So, it looks like option 2 is off the table, leaving only option 3. Now, you could set up a checked signal with

julia> s = Signal(1)
Signal{Int64}(1, nactions=0)

julia> checked = map(x->(1 <= x <= 5 || error("oops"); x), s)

but I didn't feel remotely comfortable just trusting that the user had done this: I wanted to enforce the fact that you can only create a view if you use a checked signal. In practice that seems to mean creating a specific type.

So is it the case that the video player keeps reading the SubArray object repeatedly and drawing that to the screen? I'm wondering what notifies it.

map(x->redraw!(canvas), s)

I'm slightly concerned about the dependency on OffsetArrays. I suspect that will be hard to drop. I'm fine with keeping it if so.

Note this is only a test dependency (it's in test/REQUIRE, not REQUIRE), so it's not something that gets loaded with using Reactive. I wanted to make sure it really did support all the indexing rules, but I can drop it if you prefer.

@timholy
Copy link
Member Author

timholy commented Mar 7, 2017

BTW, if you're optimistic about this, I'll look into supporting other Julia versions. There's some chance I'll advocate dropping 0.4 support unless you feel strongly about it.

@shashi
Copy link
Member

shashi commented Mar 8, 2017

What's the API for the user like? Do they input this via a slider or something?

Consider:

s=Signal(1)
foreach(data->redraw!(canvas, data), map(x->view(A, b, c, x), s))

Sorry if I missed this while reading the comment, but I'm wondering why would something like this not work out well enough? Is it because it would allocate a new SubArray on every update ? If I had to take a guess, I'd guess it might be because of the ease of use... I will agree that expecting the user to create a signal of SubArrays is not great API, which is why (along with helping achieve great performance) I'm willing to merge this change. However, the example is how Reactive (or FRP) was designed to be used. Users use it at the top-most level, and functions that build the UI and data are simple functions from non-signals to non-signals (Think of it as the . broadcast syntax. I think the recommendation is similar for it in that library code (e.g. the standard library) itself doesn't use it. The user uses it at the top-most level. On v0.6 we could even use the broadcast syntax to lift expressions to become signals. Something like @SimonDanisch's constlift`)

Arguably this is not good for library writers like you who want to use Reactive under the hood to make a media player for example. I'm thinking of alternatives to Reactive that are less opinionated than this. (Reactive can sit on top of such a thing) the closest thing I think is something like Signals and Slots from Qt (Keno has been insisting I read about it, and I finally did. Here's an experiment). I believe Simon has thought a bit about this too.

Meanwhile, feel free to merge this! Could you copy the contents of the OP into a section in the documentation?

Note this is only a test dependency

Oops sorry! I missed that. :)

@shashi
Copy link
Member

shashi commented Mar 8, 2017

There's some chance I'll advocate dropping 0.4 support unless you feel strongly about it.

Sure, that's fine! 👍

@timholy
Copy link
Member Author

timholy commented Mar 8, 2017

What's the API for the user like? Do they input this via a slider or something?

Sure. There's already a more sophisticated player widget on the dev branch of GtkReactive.

I'm wondering why would something like this not work out well enough?

Thanks for pointing this out explicitly. It's perfect for simple "throwaway" GUIs, but perhaps less so for complicated ones. Let's say I've gone to the effort to create a sophisticated viewer (implements zoom, handles pixels with aspect ratios different from 1, allows you to adjust contrast, etc), but that I've implemented it only for 2d. Now I have a 3d array and want to leverage this for visualizing the xy plane in one panel, the xz plane in a second panel, and the yz plane in a third panel. If I do it the way you're suggesting, I have two options:

  1. Rewrite so it can accept a 3d array as an input, plus another argument to specify which plane you want to visualize (in other words, which argument of view should the signal be stuffed into). I've been that route before (ImageView), and while it works (and should be supported) I don't think it's ideal. Among other things, you have to come up with an axis-naming convention unless the user is exploiting, e.g., AxisArrays.jl, and generally it's not possible to make those operations type-stable. (May not be much need, however.) More significantly, unless you apply that choice right at the beginning, you end up having to propagate the plane-selection variables throughout your code, which makes it messy. You can't apply that choice statically, however, because it breaks the ability to perform the kind of manipulation we're talking about here. That brings up the next option...
  2. Write a special dispatch version myviewer(imgsignal::Signal{<:AbstractMatrix}) and pass map(z->view(A, :, :, z), s) as imgsignal.

Overall I like option 2 far better. But we have a lot of numerical code in Base and elsewhere that doesn't accept a Signal input but does accept an AbstractArray input. Yes, I could use map(mean, imgsignal) to calculate the mean value, but as you say that ends up creating new objects each time I do it and that has performance consequences (most problematic for small objects):

 julia> A = rand(3,3,3);

julia> using Reactive

julia> s = CheckedSignal(1, 1:3)
Reactive.CheckedSignal{Int64,UnitRange{Int64}}(Signal{Int64}(1, nactions=0),1:3)

julia> As = view(A, :, :, s)
3×3 SubArray{Float64,2,Array{Float64,3},Tuple{Colon,Colon,Reactive.CheckedSignal{Int64,UnitRange{Int64}}},false}:
 0.237056  0.394345  0.304798
 0.751526  0.689016  0.378245
 0.680932  0.180326  0.459476

julia> ss = Signal(1)
Signal{Int64}(1, nactions=0)

julia> Asig = map(z->view(A, :, :, z), ss)
Signal{SubArray{Float64,2,Array{Float64,3},Tuple{Colon,Colon,Int64},true}}([0.237056 0.394345 0.304798; 0.751526 0.689016 0.378245; 0.680932 0.180326 0.459476], nactions=0)

julia> mean(As)
0.45285787935423016

julia> map(mean, Asig)
Signal{Float64}(0.45285787935423016, nactions=0)

julia> V = view(A, :, :, 1)
3×3 SubArray{Float64,2,Array{Float64,3},Tuple{Colon,Colon,Int64},true}:
 0.237056  0.394345  0.304798
 0.751526  0.689016  0.378245
 0.680932  0.180326  0.459476

julia> using BenchmarkTools

julia> @benchmark mean($V) seconds=1
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     10.798 ns (0.00% GC)
  median time:      11.857 ns (0.00% GC)
  mean time:        11.797 ns (0.00% GC)
  maximum time:     30.170 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     999
  time tolerance:   5.00%
  memory tolerance: 1.00%

julia> @benchmark mean($As) seconds=1
BenchmarkTools.Trial: 
  memory estimate:  96 bytes
  allocs estimate:  3
  --------------
  minimum time:     72.317 ns (0.00% GC)
  median time:      76.183 ns (0.00% GC)
  mean time:        107.123 ns (19.06% GC)
  maximum time:     7.479 μs (97.61% GC)
  --------------
  samples:          9231
  evals/sample:     974
  time tolerance:   5.00%
  memory tolerance: 1.00%

julia> @benchmark map($mean, $Asig) seconds=1
BenchmarkTools.Trial: 
  memory estimate:  880 bytes
  allocs estimate:  13
  --------------
  minimum time:     4.475 μs (0.00% GC)
  median time:      4.824 μs (0.00% GC)
  mean time:        6.394 μs (11.15% GC)
  maximum time:     2.801 ms (99.68% GC)
  --------------
  samples:          10000
  evals/sample:     7
  time tolerance:   5.00%
  memory tolerance: 1.00%

So with the strategy here we get something that's at least within spitting distance of "static" views, but the penalty is well over 50x when we map the array as a whole.

I think the main advantage of this strategy is it still walks, talks, and acts like an AbstractArray.

I'm thinking of alternatives to Reactive that are less opinionated than this. (Reactive can sit on top of such a thing) the closest thing I think is something like Signals and Slots from Qt (Keno has been insisting I read about it, and I finally did. Here's an experiment).

A big 👍 from me for this plan. I don't fully understand why you store the listeners in a global dictionary, though; why not store them in a vector in the object itself? That too will allow the object to be GCed. Moreover, it seems better to store the listeners for an object in a Vector{WeakRef} or WeakKeySet (aka, WeakKeyDict{Any,Void}) rather than a Vector{Any}, so that individual listeners can also be GCed.

But on general principle, I think that would cover my needs nicely, with two additions:

  • more utilities, like the ability to disconnect a specific listener
  • while I strongly prefer a synchronous execution model for most uses, some asynchronous (queued) facility seems very useful (e.g., throttle is very nice for canvas redraws).

Can we split that out into it's own small package divorced from web programming? ObserverPattern.jl?

@timholy
Copy link
Member Author

timholy commented Mar 8, 2017

@shashi
Copy link
Member

shashi commented Mar 20, 2017

Sorry for taking forever to reply. @timholy now that you have access do merge this as you see fit :)

I suspected those performance numbers would turn out that way.

I don't fully understand why you store the listeners in a global dictionary

lol that is kinda dumb. I don't know why that was my first impulse here. Will change it to be a field.

so that individual listeners can also be GCed

I was planning on having a way to remove a listener explicitly.

A big 👍 from me for this plan.
Thank you for the encouragement!

ObserverPattern.jl?

I wasn't sure if I'll need WebIO specifics in the code. I might need a string identifier field though. Maybe we can take it out into a package.

@timholy
Copy link
Member Author

timholy commented Mar 20, 2017

I've not merged because I'm taking to heart the comment of how Reactive is intended to be used. Now that I've spent more time playing with it (JuliaGizmos/GtkReactive.jl#2), I see what you mean. I wonder if this PR might be more appropriate for a SignalsSlots/ObserverPattern based implementation.

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