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

Some helper functions #150

Open
pevnak opened this issue Nov 24, 2020 · 5 comments
Open

Some helper functions #150

pevnak opened this issue Nov 24, 2020 · 5 comments

Comments

@pevnak
Copy link

pevnak commented Nov 24, 2020

Dear All,

I am using setfield for indexing in terribly complicated tree structures naturally present in tree data. Since I frequently need programmatically adapt lenses, it helps a lot, if they are in normalized form. I have therefore made a few functions, which simplifies my life a lot and I wonder, if that would be something you would consider interesting.

The first function "normalizes" lenses, such that outerlens in CompoundLens is simple.

lenskeys(lens::Setfield.ComposedLens) = vcat(lenskeys(lens.outer), lenskeys(lens.inner))
lenskeys(lens::Setfield.PropertyLens{K}) where {K} = [K]

function normalizelens(lens::Setfield.ComposedLens)
  ls = lenskeys(lens)
  mapfoldr(k -> Setfield.PropertyLens{k}(), ∘, ls)
end

And when I have the lenskeys, I can easily implement indexing as

function Base.getindex(lens::Setfield.ComposedLens, i...)
  ls = lenskeys(lens)
  mapfoldr(k -> Setfield.PropertyLens{k}(), ∘, ls[i...])
end

Base.lastindex(lens::Setfield.ComposedLens) = length(lenskeys(lens))

I would like to know your opinion and if they would fit to the library. If so, I would prepare a full PR.

Thanks a lot,
Tomas

@jw3126
Copy link
Owner

jw3126 commented Nov 24, 2020

Hi @pevnak
glad you are using Setfield.jl. We can include some form of this functionality, but I have some objections to the exact proposal.

  • It is not type really stable. This might be ok for your application, but here we try to make everything usable in the hot loop if possible.
  • It should go to Accessors.jl. Setfield.jl is still maintained, but new functionality should go to Accessors.jl.

What I am happy to include is some helper like this:

using Accessors
import Accessors: ComposedOptic

decompose(optic) = (optic,)
decompose(optic::ComposedOptic) = (decompose(optic.outer)..., decompose(optic.inner)...)

A PR to Accessors.jl would be welcome!

You could then do the indexing like ∘(decompose(@optic(_.a.b.c))[[1,3]]...). Note that this is also much more general and not restricted to PropertyLens.

@jw3126
Copy link
Owner

jw3126 commented Nov 24, 2020

Actually I think the decompose function can even go to CompositionsBase.jl.

@pevnak
Copy link
Author

pevnak commented Nov 25, 2020

I like the decompose a lot. It is nicer and cleaner over what I have written. The indexing is also very nice.

To which part you refer as not being type stable? The normalization was mean to ensure that the outer part of ComposedLens is simple, which I think is not against the type stability. In my ideal view, the output of a composition would be a normalized Lens, but that might not be compatible some people use cases.

I can create these PRs with Tests to Accessors.jl.
What should be the part of it?

  • decompose
  • indexing
  • normalization

@jw3126
Copy link
Owner

jw3126 commented Nov 25, 2020

  • You can add decompose to [CompositionsBase.jl](https://github.com/JuliaFunctional/CompositionsBase.jl). That way it is useful to even more people. The direction Lens -> Vector{Symbol} is type stable. But there is no type stable way back.

  • You can also add to some helper like propname(::PropertyLens{name}) where {name} = name to Accessors.jl. So lenskeys becomes map(propname, decompose(lens)).

  • It would also be nice to have normalization in Accessors.jl. Note that the composition order influences performance, so normalization should give the fast order.

  • I would prefer not to have indexing. I think it's usefulness is in a too limited situation to justify haveing it in Accessors.

@pevnak
Copy link
Author

pevnak commented Nov 25, 2020

This is crystal clear answer.

I will work on relevant PRs.

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

2 participants