add setall()

[aplavin]

Copy-pasted till depth=4, seems to infer fine. The plan is to make the same kind of macro as with getall in the end, after all other questions are resolved.

The implementation doesn't seem very clean, though - unlike getall. Improvement suggestions are welcome!
Also, it's likely not the most efficient: getall is called more times than strictly needed. Would be great to remove extra calls, the complication is that we don't want setall to recurse: currently, it only calls setall with individual (not composed) optics.

[jw3126]

Thanks a lot for giving this a shot!

For me personally, the really useful cases would be, where getall and setall work with Vector, while your PRs focus on the Tuple case.

Implementation wise, I might have tried to use less getall. I think in many places
where you call getall you really only need to know length(getall(...)) or maybe Val(length(getall(...))). So maybe it makes sense to define a length_getall that just computes this without actually forming unneeded intermediate getall results?

OTHO I find it hard to reason about the optimizer and inference here so not sure if this would actually improve things.

[aplavin]

For me personally, the really useful cases would be, where getall and setall work with Vector, while your PRs focus on the Tuple case.

I also find vectors important here, and getall already returns them when e.g. there are vectors among requested fields.
When possible, it does return a Tuple though: a uniform Tuple can be turned into a Vector almost for free, but not the other way around.

Ideally, setall should support vectors/tuples/any collection as the values-to-set. This is fundamentally type-stable when the Vector is concretely typed, and otherwise it should work nevertheless.

Do you have some else in mind?

So maybe it makes sense to define a length_getall that just computes this without actually forming unneeded intermediate getall results?

Haven't tried that, but most likely it has to be yet another wannabe-recursive function generated with macros...

I'm also thinking that maybe some insights from #23 development could be useful here? @rafaqz

[jw3126]

Ideally, setall should support vectors/tuples/any collection as the values-to-set. This is fundamentally type-stable when the Vector is concretely typed, and otherwise it should work nevertheless.
Do you have some else in mind?

There are some optimizations one can do in the vector case, but that does not have to be in this PR. Like using views for splitting in setall and maybe having a BangBang style getall!! implementation that uses append! for vectors internally.

Haven't tried that, but most likely it has to be yet another wannabe-recursive function generated with macros...
I'm also thinking that maybe some insights from #23 development could be useful here?

One issue I remember with recursion is this: JuliaLang/julia#43296 (comment)
One can workaround it by defining optic style on both the type and the instance:

OpticStyle(::Type{<:Optic}) = ...
OpticStyle(::Optic) = ...

and only call on instances instead of types whenever possible.

[aplavin]

using views for splitting in setall

Makes sense, and I think it would be easy to do once the basic indexing setall works fine.

having a BangBang style getall!! implementation that uses append! for vectors internally

Curious what usecases you foresee for this, and what the semantic should be. Can it even be defined unambiguously for nontrivial cases? Like, a |> Elements() |> Elements().

[jw3126]

Curious what usecases you foresee for this, and what the semantic should be.

The semantics would be that getall!!(out, obj, optic) is equivalent to append!!(out, getall(obj, optic)). That is it concatenates out with getall(obj, optic) by mutation if possible or in a functional way otherwise.

The main use case of getall!! is probably internal, namely to implement getall. In general, you can use getall!!, when you getall! allocates too much. Also it can be hard for the compiler to infer the output type and passing outexplicitly can help with that. Typically the user would pass an emptyout. But non empty out can be useful, for instance to implement getallin terms ofgetall!!`.

Can it even be defined unambiguously for nontrivial cases? Like, a |> Elements() |> Elements().

Can you give an example what potential ambiguity you see?

[aplavin]

Sorry, I misread and thought you are talking about setall!!.
And yes, this getall!! was definitely out of scope for my recent getall PR.

[bgroenks96]

I am a bit confused as to the relationship between this PR and getall to what is implemented in #23. Could someone clarify this?

[jw3126]

I am a bit confused as to the relationship between this PR and getall to what is implemented in #23. Could someone clarify this?

The compiler does not like the kind of recursion used in #23. The PRs here infer better but don't allow recursion. For instance you can't get all Float64 fields burried in an arbitrary nested struct with this PR.

[aplavin]

These PRs and the potential differences were discussed in #63.

@jw3126 touched on the implementation side, but there's also a design difference.
In my opinion, "Query" from #23 shouldn't be yet another independent optic. Instead, all optics should support getall and setall functions, to extract all values as a flat tuple/vector, or set all values from a tuple/vector. This way is more general and composable between different optics.
Then the usecase of "extracting all arbitrarily nested Float64 values" is effectively getall(obj, Recursive(...)), potentially with some extensions of the Recursive optic.

Yes, for now the (already released) getall PR only supports non-Recursive optics, same with this PR. This is the implementation difficulty mentioned by @jw3126. And as you can see, this setall PR kinda stalled, because setall is even more complicated to make efficient. Maybe, some tricks from #23 can be employed, as well as hoping for compiler improvements...

[bgroenks96]

But you are able to make it type stable for up to 4 levels of nesting currently? Could this be extended further? There probably is a reasonable limit (maybe <10) that would cover 99% of real world use cases.

[aplavin]

That's exactly what we did with getall :)

Code in this PR is indeed typestable, but still not very efficient. It calls getall way too many times than should be necessary, I think

[jw3126]

I think there is another subtle issue here. What is type stable here are cases, where the recursion depth is "obvious". Like
Properties() |> Properties() |> Properities() is depth 3. But cases like Recursive(..., Properties()) are hard for inference even if you only use them on depth 3 structs I think.

[aplavin]

That's true, Recursive is specifically out of scope here due to compiler not being that smart. For me this is totally fine, as most getall/setall usecases are with plain non-recursive (but nested) optics.
When the compiler improves or someone figures the proper solution, as was attempted in #23, Recursive can be added as well.

[bgroenks96]

You may want to look at how this problem was solved in Flatten.jl reconstruct. I used generated functions to recursively construct a double-linked tree data structure that allowed reconstruct to generate code at compile-time that would reconstruct any nested concrete type. It's messy, but it works. And as far as I know, it is still type-stable even on Julia 1.8.

[aplavin]

See my new update:

• getall implementation simplified
• type-stable and pretty reasonable (I think) setall implementation

Comments and suggestions welcome!

[aplavin]

I really like how neatly everything works together:

julia> using Accessors, ForwardDiff, StaticArrays

# arbitrary function that takes an object
julia> f(x) = exp(x.a.b) - x.a.c

# differentiate wrt this part of x
julia> o = @optic _.a |> Properties()

# at this value of x
julia> x0 = (a=(b=1, c=2),)

julia> @btime ForwardDiff.gradient(v -> f(setall($x0, $o, v)), SVector(getall($x0, $o)))
  12.127 ns (0 allocations: 0 bytes)
2-element SVector{2, Float64} with indices SOneTo(2):
  2.718281828459045
 -1.0

Will probably post in #appreciation after this gets released :)

[jw3126]

LGTM thanks a lot @aplavin this is awesome!