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

get/set all values referred by an optic #63

Closed
aplavin opened this issue Jul 13, 2022 · 39 comments
Closed

get/set all values referred by an optic #63

aplavin opened this issue Jul 13, 2022 · 39 comments

Comments

@aplavin
Copy link
Collaborator

aplavin commented Jul 13, 2022

Some existing optics, such as Elements(), If() and Recursive(), can only be modifyed - one cannot retrieve the values specified by them. I understand that it's not obvious how to implement optic(obj) for them and even what type it should return. However, from time to time I try to reach for this functionality: either to specifically extract the corresponding values, or just to see what values modify would be called with.

Do you think it's possible to design value extraction for those optics in a consistent manner?

@jw3126
Copy link
Member

jw3126 commented Jul 13, 2022

A quick and dirty way to do this is:

out = Any[]
modify(obj, optic) do val
    push!(out, val)
    f(val)
end

@rafaqz has done a lot of work in this direction, see #23
Ultimately we could not find a performant way to implement this, that's why the PR is not merged.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 13, 2022

#23 looks a bit over my head - not even talking about implementation, I don't fully understand the interface and usecases.
Extracting values with Elements and If is probably much easier to implement performantly than very general #23 queries. A major issue is the interface: it should be consistent with

function test_modify_law(f, lens, obj)
obj_modify = modify(f, obj, lens)
old_val = lens(obj)
val = f(old_val)
obj_setfget = set(obj, lens, val)
@test obj_modify == obj_setfget
end

@jw3126
Copy link
Member

jw3126 commented Jul 13, 2022

Okay, so is your question whether one can implement optic(obj) for these such that the lens laws are satisfied?

I think that is not really possible. For instance:

julia> using Accessors

julia> obj = [1,2,3]
3-element Vector{Int64}:
 1
 2
 3

julia> optic = Elements()
Elements()

julia> modify(identity, obj, optic)
3-element Vector{Int64}:
 1
 2
 3

julia> set(obj, optic, 1)
3-element Vector{Int64}:
 1
 1
 1

That means you can only ever construct constant vectors using set, Elements(), but you can produce arbitrary vectors using modify, Elements(). So there is no hope to decompose modify into set and "get".

Mathematically there are many flavors of optics and these can have different APIs and laws. Lens is one with lots of structure and laws. Elements() only satisfies the functor laws and does not have a "get" operation:

  • @assert modify(identity, optic, obj) == obj
  • @assert modify(f ∘g, optic, obj) == modify(f, optic, modify(g, optic, obj))

@rafaqz
Copy link
Member

rafaqz commented Jul 13, 2022

#23 looks a bit over my head - not even talking about implementation, I don't fully understand the interface and usecases.
Extracting values with Elements and If is probably much easier to implement performantly than very general #23 queries. A major issue is the interface: it should be consistent with

function test_modify_law(f, lens, obj)
obj_modify = modify(f, obj, lens)
old_val = lens(obj)
val = f(old_val)
obj_setfget = set(obj, lens, val)
@test obj_modify == obj_setfget
end

The use case is reconstructing arbitrary immutable objects with new/modified field values.

https://github.com/rafaqz/ModelParameters.jl is the best example of how powerful this can be.

You can also use it to e.g. replace Arrays deeply nested in wrapper types with GPUArrays. You dont need to know or specify the fields, just the types to replace.

You can set values using this method, by passing in a tuple. But the order of the matched field types in the object tree determines the order objects are replaced in.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 13, 2022

The use case is reconstructing arbitrary immutable objects with new/modified field values.

That describes the whole Accessors.jl :)

With simple lenses in Accessors, it's pretty clear when to use them and what's the benefit. More complex optics, such as Elements(), require some getting used to, but they remain relatively transparent. I personally never used Recursive(), which is probably the most advanced optic here - just never encountered a reasonable usecase myself.

You can also use it to e.g. replace Arrays deeply nested in wrapper types with GPUArrays. You dont need to know or specify the fields, just the types to replace.

That sounds like a job for the Recursive() optic, right? Descend wherever possible and process what you see.
What would #23 bring on top of it?

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 13, 2022

@jw3126 thanks for the detailed explanation!

Do you think there's space for functions like get_all(obj, optic) and set_all(obj, optic, values) that extract all specified values as some sort of collection? Other than Recursive, this shouldn't be too hard to implement efficiently, I think.

Optics have great composability, so that one learns about a few concepts and they (often) seamlessly combine together. Hoping for something similar with "multivalued" optics, and a function like get_all...

@jw3126
Copy link
Member

jw3126 commented Jul 13, 2022

If you manage to implement a fast get_all that would be super awesome. In #23 we were not able to implement a get_all that type inference liked.

@rafaqz
Copy link
Member

rafaqz commented Jul 13, 2022

That describes the whole Accessors.jl :)

Maybe you miss the part about arbitrary objects. Most of Accessors works on objects where you know a field name or some other detail. But sometimes we don't know anything about the structure of the objects or the names of any of fields, and want to make generic changes to it.

Maybe its already possible with accessors? But Im not sure how.

If you can define a function that takes the Float64 from any bits type object of any nesting depth and makes them all Float32 in the reconstructed object or a flat Tuple - and any other similar transformation - calculated at compile time. That would remove the need for that PR.

How youre describing get_all and set_all is essentially what my PR does, and it composes with other optics.

Edit: I may not have been clear but being able to get/set objects from a Tuple is a key part of that PR, rather than just replacing things as you walk an object. Some other package like Optim.jl may be in the middle of the transformation wanting a vector of floats, so Recursive is not going to be enough.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

Yes, I understand the usecase "replace all values of this type to this type" now: like the GPU conversion or Float bitness.
Probably, I even wanted something like this, once or twice. Getting (or setting?) all values specified by an optic is a more common case, regularly wish this existed.

How youre describing get_all and set_all is essentially what my PR does, and it composes with other optics.

Looked once again into that PR, still not sure if I fully understand the interface. What do you mean "composes with other optics"? When would one use Query with another optic?

I would say a more intuitive approach wouldn't even involve introducing a new type: just define getall/setall on existing optics, including Recursive(). Of course, this entails the same type stability issues (hopefully will resolve in a later Julia?), but here I'm only talking about the interface.

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

I dont realy get what is different with your proposal to that PR or what you dont understand at this point 😆

It has to be able to replace Flatten.jl and power ModelParameters.jl, as I linked earlier. That was my main goal, with some nice @setall syntax on top.

By composes I mean you could do something like @setall of the val fields of every United.Quantity in an object, using an optic. I cant remember the syntax to do that, but there are examples in the thread and the tests for the PR.

Maybe you should try it out rather than reinvent that wheel, if you will likely have the same type stability issue.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

Suppose I have an optic like opt = @optic _.a |> Elements() |> _.b |> Properties(). Does Query provide a way to get all elements referred to by this optic, as in getall(obj, opt)? That, and the corresponding setall, is basically everything I'd need.

I haven't tried implementing getall yet, do you think the type stability issues remain even without arbitrary recursion? I mean limiting getall to simple lenses and optics like Elements or Properties - for now.

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

I don't know how Elements works really, but you can do things like this with arbitrary optics where _[2].g is:

@getall (x[2].g for x in missings_obj if x isa NamedTuple)
@setall (x[2].g for x in missings_obj if x isa NamedTuple) = 5

This example gets all theg properties of the second value in every NamedTuple in any object , or sets them all to equal 5.

You can do this manually without the macros using Query and passing it your optic.

See:
https://github.com/rafaqz/Accessors.jl/blob/queries/test/test_queries.jl

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

A major point of composability is the ability to just pass an optic, and let the inner function do whatever it wants with the values specified by that optic. Currently, the values can always be modified, sometimes can be get and set. My proposal is to extend this interface with getall and setall -- for all optics!

Query seems to solve a somewhat different, arguably more specialized, problem. I don't really want to recurse indefinitely and find all NamedTuples!
And again, I believe it doesn't require adding yet another optic -- the current set seems rich enough to cover the example usecases.

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

"Get me every Float64 in this thing" seems like incredibly generic functionality to me ;)

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

Sorry, do you agree with something, or arguing with something from my previous message?

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

I mean its not really worth arguing over which appoach is more general or "arguably more specialised", Im not sure why that's relevent.

I have never needed the thing that you want.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

Maybe a poor choice of the words on my end. Of course, "Get me every Float64 in this thing" is generic and works with all kinds of objects, if properly implemented.
But getall/setall for all optics is objectively more general than that!
Need all floats no matter where they are? Use getall with Recursive(). It's main purpose is exactly to search a nested objects by criteria.
Need something simpler, like extract (1, 3) from obj = (a=((x=1, y=2), (x=3, w=4)), b=(5, 6))? Use getall(obj, @optic _.a |> Elements() |> _.x).

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

Ok hah. Maybe we are talking past each other?

Sure, your proposal is generic in relation to other optics. But not in terms of the interaction with other packages, e.g. like Optim.jl.

Getting and setting everything as a flat tuple is a nice generic interface for the rest of the ecosystem. It removes all structure and complexity of objects.

Thats the reason Query is written like it is rather than using Recursive. Its the reason Flatten.jl exists: to create a simple interface between complex objects and all the use cases for flat vector/tuple inputs, that are required by many packages.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

But not in terms of the interaction with other packages, e.g. like Optim.jl.

Hm, why?.. My getall/setall proposal seems to provide strictly more functionality than your examples in this thread need.

Getting and setting everything as a flat tuple is a nice generic interface for the rest of the ecosystem.

Sure, that's great and can be useful, as we discussed above.
And this is literally what getall is supposed to do.

See the example with Recursive(), that modifies all numbers in an arbitrarily nested object:

julia> obj = (x=1,y=2,z=(w=3,u=(a=4,b=5),v=6))
(x = 1, y = 2, z = (w = 3, u = (a = 4, b = 5), v = 6))

julia> modify(x -> 100x, obj, Recursive(x -> !(x isa Number), Properties()))
(x = 100, y = 200, z = (w = 300, u = (a = 400, b = 500), v = 600))

Currently, only modify works with Recursive. A potential future getall(obj, Recursive(...)) would return all these numbers as a flat tuple - exactly what you need (right?).

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

I have never needed the thing that you want.

That's quite surprising for me, actually, considering that you work with nested models and their parameters.
Let's say a have a model with a couple of components:

struct CompA
     x
     y
     ...
end

struct CompB
     x
     y
     ...
end

struct MyModel
    comp_a::CompA
    comps_b::Tuple{CompB...}
end

m = MyModel(CompA(...), (CompB(...), CompB(...)))

Now, I need to change (manually/with optimizer/with sampler/whatever) some parameters.
Want x and y in CompA?

o = @optic _.comp_a[(:x, :y)]

tup = getall(m, o)
... work with tup ...
new_m = setall(m, o, new_tup)

Want x and y of all CompBs? o = @optic _.comps_b |> Elements() |> _[(:x, :y)], all code using o stays the same.
All properties of all CompBs? o = @optic _.comps_b |> Elements() |> Properties().

Currently, I use tuples of lenses, and "broadcast" their get/set methods to attain such behavior. However, this only works with scalar lenses -- not Elements(), not Properties().

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

But how does it set all the numbers in an object from a flat tuple?

Setting is the hard part, getting is clearly trivial.

(Your example is also trivial right? The problem is when you dont know any of the field names or the structure of what youre working with - im writing generic code for other peoples models, not bespoke methods for my own)

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 14, 2022

Setting is the hard part, getting is clearly trivial.

Do you say this from the interface/API perspective, or regarding implementation?

The former seems clear: setall(x, opt, getall(x, opt)) should return something like the original x, so setall just sets all values in the same locations and in the same order as getall retrieves them.

As for implementation, as I understand neither getting nor setting can be done inferrably now. Is that not the case?

@rafaqz
Copy link
Member

rafaqz commented Jul 14, 2022

Getting can be inferrable and is in Flatten.jl, and get/set used to compile away in Flatten.jl before compiler changes killed that.

Its still some nanoseconds (like ~50 maybe fir something small? I dont remember) in the query PR here and can compile away when you use Revise.jl (a compiler bug). But not without it, and some use cases have a hard requirement that it compiles away.

Setting is harder in terms of implementation because you need to pass the state of your iterator around as you walk the tree. The leaves need to know about their position in the tree somehow. Getting doesnt need that, the order is implicit.

@jw3126
Copy link
Member

jw3126 commented Jul 15, 2022

Setting is the hard part, getting is clearly trivial.

Mhh I think getting is already non trivial IIRC in #23 getall was not inferrable either.

@rafaqz
Copy link
Member

rafaqz commented Jul 15, 2022

But the code is much simpler. Its only a few lines in Flatten.jl and it it compiles away. You also dont have the problem in of iterating the set tuple somehow.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 16, 2022

Setting is the hard part, getting is clearly trivial.
(Your example is also trivial right?

What do you mean by "trivial" here? I'm not aware of any existing function that can extract all values referenced by an optic.

The problem is when you dont know any of the field names or the structure of what youre working with - im writing generic code for other peoples models, not bespoke methods for my own)

Note that my proposal is strictly more generic than that! getall/setall(obj, optic) apply both when the structure is partially known (arguably the most common case), as well as when it's unknown when writing the code.

With #23 as-is, there is still no way to extract values defined by an optic (right?).
OTOH, if getall/setall are defined on existing lenses instead of introducing Query, one has both. getall(obj, optc) for getting all referenced values, and something like getall(obj, Recursive(x -> !(x isa Number), Properties())) to get all numbers no matter where they are. Same with setall.

Moreover, the implementation can stay almost the same as #23. I think, what's implemented there is effectively getall/setall(Recursive) - the most difficult part. Remaining getall/setall on single-value lenses and Elements/Properties are clearly simpler.

@rafaqz
Copy link
Member

rafaqz commented Jul 16, 2022

I dont think that its actually the same as setall on Recursive? Recursive returns nested tuples, not flat tuples.

Doesn't that mean it would set from nested tuples too? How is it a superset of Query?

To use flat tuple inputs we will still need some Query-like function to build the nested tuples to pass to Recursive.

A more minor difference is Recursive only has a descent condition. Query also has a select condition - objects you take without descending into them, so the descend condition lets you ignore some objects completely.

Recursive should totally do what you want, but I cant see how it replaces Query.

By trivial I mean in the development of Flatten.jl the getall analog flatten was a very small fraction of dev time, and it compiles away. setall reconstruct was the majority of it and it still doesnt.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 16, 2022

Recursive returns nested tuples, not flat tuples.

Are you talking about the current situation, or my get/setall proposal? Currently, Recursive only supports modify, not get/set - so it doesn't really "return" anything.
Following my proposal, get/setall would always work with flat tuples, for all optics.

@rafaqz
Copy link
Member

rafaqz commented Jul 16, 2022

Ok right of course it doesnt. I dont actually use Recursive. But as far as I know there's nowhere in Accessors.jl that flattens anything now.

But if you can handle distributing a flat tuple through all composed optics and setting the right part of the object with the right values of the tuple that will be great.

It might be easiest to not pass around the iterator as in Query and instead just to call getall at every iteration, see how long the tuple it returns is, and pass a tuple that long with setall.

One last problem I haven't mentioned is that Query also only reconstructs objects when there were changes to their contents, to avoid trying to construct objects that dont implement constructorof. It's important for setall to not be overly fragile in the real world julia ecosystem, its the main downfall of Flatten.jl.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 16, 2022

But as far as I know there's nowhere in Accessors.jl that flattens anything now.

Sure, there isn't anything like that yet. Did you see a comment of mine above:

Moreover, the implementation can stay almost the same as #23. I think, what's implemented there is effectively getall/setall(Recursive) - the most difficult part. Remaining getall/setall on single-value lenses and Elements/Properties are clearly simpler.

So, the implementation of getall/setall(Recursive) can be effectively the same code as you have in that PR.

One last problem I haven't mentioned is that Query also only reconstructs objects when there were changes to their contents, to avoid trying to construct objects that dont implement constructorof.

Interesting, that can also be useful for performance if constructors exist but perform some checks.

I may try implementing getall/setall for some non-recursive optics for now. Hope that they won't suffer from inference issues that much...

@rafaqz
Copy link
Member

rafaqz commented Jul 16, 2022

The inference problem is specifically with recursion, so you should be fine.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 16, 2022

Recursion is there with CompositeOptics as well, though. But I'll try :)

@rafaqz
Copy link
Member

rafaqz commented Jul 16, 2022

Possible using the new 1.8 @assume_effects macro will solve all of these problems. The problem for us will be splitting the compile time and runtime conditions - x isa Int is very different condition to x == 5 for the compiler, but we are treating them as the same. We can force the first to compile away with @assume_effects, but not the second.

@aplavin
Copy link
Collaborator Author

aplavin commented Jul 16, 2022

x == static(5) may help to push the latter condition into types, if that turns out useful.

See the linked PR, I tried adding simple getall methods for now. They work, and everything is type-stable in principle, but long optics chains don't fully infer.

@rafaqz
Copy link
Member

rafaqz commented Jul 16, 2022

It won't because the value in the struct is still determined at runtime.

I think we need the opposit - a separate path for descend to just check types match rather than calling a function that returns a Bool. Then we can use @assume_effects on that path and force it to compile.

@aplavin aplavin mentioned this issue Aug 22, 2022
@aplavin aplavin changed the title modify-only optics get/set all values referred by an optic Jan 25, 2023
@aplavin
Copy link
Collaborator Author

aplavin commented Jan 25, 2023

Given the discussion in this thread and following get/setall PRs, I updated the title to be more relevant.

Briefly, the current state is that getting/setting all referred values works for all optics with the exception being Recursive.
It's challenging to make Recursive getall/setall zero or low cost. It requires either improvements to the Julia compiler, or some clever trick, and/or making a more specialized recursive optic. For example, RecursiveOfType(Float64, Properties()).

@aplavin
Copy link
Collaborator Author

aplavin commented Jan 31, 2023

Regarding compiler improvements: JuliaLang/julia#48059 looks relevant. Do you @jw3126 @rafaqz think that change would allow type-stable "get-all-from-Recursive", or something else is needed?

@jw3126
Copy link
Member

jw3126 commented Jan 31, 2023

One issue that I am not sure is addressed by this PR would be
JuliaLang/julia#43296 (comment)

@aplavin
Copy link
Collaborator Author

aplavin commented Apr 23, 2023

getall(Recursive) could be supported by #98, setall(Recursive) is complicated, if possible at all.

But see RecursiveOfType now available in AccessorsExtra. Eg, RecursiveOfType(Number) is an optic targeting all numbers in a struct, arbitrarily nested. RecursiveOfType supports all three operations, modify/getall/setall, and should cover the vast majority of relevant usecases.

See basic examples here, and how it transparently works with Optimization here.

@aplavin aplavin closed this as completed Jun 8, 2023
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

No branches or pull requests

3 participants