Setfield.jl's `IndexLens` does not use views

[torfjelde]

One drawback with adopting the use of Setfield.jl's lenses for VarName is that we're no longer using view when we're indexing into arrays.

This could easily be fixed by just implementing a ViewLens by copy-pastingSetfield.IndexLens and using a view instead of getindex.

Sooo should we?

EDIT: This might also be a APPL issue rather than DPPL. What say you @phipsgabler ?

[phipsgabler]

I think this is a reasonable idea in principle, but am not sure about the design. THB i'd even say it's a Setfield.jl issue.

Anyway. A ViewLens type like you describe would be a valid lense in the loose sense (get(set(obj, l, val), l) == val, but not of the same type). A view instead of the same array type is a valid representation type in the general optics perspective.

This felt a bit weird to me, thinking about it for the first time. What about getter and setter functions, just like view complements getindex?

julia> Base.@propagate_inbounds function getview(obj, l::Setfield.IndexLens)
           Base.maybeview(obj, l.indices...)
       end
getview (generic function with 1 method)

julia> getview(obj, l::Setfield.Lens) = get(obj, l)
getview (generic function with 2 methods)

julia> Base.@propagate_inbounds function getview(obj, l::Setfield.ComposedLens)
           inner_obj = getview(obj, l.outer)
           getview(inner_obj, l.inner)
       end
getview (generic function with 3 methods)

julia> x
10-element Vector{Int64}:
  1
  2
  3
  4
  5
  6
  7
  8
  9
 10

julia> l = Setfield.IndexLens((1:3,))
(@lens _[1:3])

julia> getview(x, l)
3-element view(::Vector{Int64}, 1:3) with eltype Int64:
 1
 2
 3

julia> function setview(obj,l::Setfield.ComposedLens, val)
           inner_obj = getview(obj, l.outer)
           inner_val = setview(inner_obj, l.inner, val)
           setview(obj, l.outer, inner_val)
       end
setview (generic function with 1 method)

julia> setview(obj, l::Setfield.Lens, val) = set(obj, l, val)
setview (generic function with 2 methods)

julia> setview(x, l, [0, 0, 0])
10-element Vector{Any}:
  0
  0
  0
  4
  5
  6
  7
  8
  9
 10

On the second thought, ViewLens plus a function views(::Lens) which recursively transforms IndexLenses seems elegant, too. It just has the disadvantage that then the VarInfo keys would consist of non-standard lenses, and indexing would always result in a view. (Unless we keep the IndexLenses, but always apply views before get internally; but that seems like unnecessary work.)

[torfjelde]

Good points all around; I'm now in favour of the getview idea instead 👍 In particular because it avoids the issues with the keys in the varinfo.

[phipsgabler]

Great. It also avoids this issue which would occur when you have both ViewLenses and IndexLenses, even if that'll be fixed soon.

[yebai]

We have switched to Accessors.jl