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

Decomposition and normalisation #10

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

Decomposition and normalisation #10

wants to merge 3 commits into from

Conversation

pevnak
Copy link

@pevnak pevnak commented Nov 28, 2020

Hi,

these are implementations of those two functions we have discussed in the Setfield thread.
They works as

julia> optic = (@optic _.c) ∘ (@optic _.a.b)
(@optic _.c) ∘ (@optic _.b) ∘ (@optic _.a)

julia> optic.inner
(@optic _.b) ∘ (@optic _.a)

julia> optic.outer
(@optic _.a)

julia> normalise(optic).inner
(@optic _.a)

julia> normalise(optic).outer
(@optic _.c) ∘ (@optic _.b)

and

julia> decompose(@optic _.a.b.c.d)
((@optic _.d), (@optic _.c), (@optic _.b), (@optic _.a))

I have add docs and tests.

@jw3126
Copy link
Member

jw3126 commented Nov 29, 2020

Thanks a lot @pevnak ! What do you think @tkf ?

  • I think decompose should go to CompositionsBase. What it does is
    decompose(sin ∘cos ∘ tan) === (sin, cos, tan).

  • normalize does normalize((sin ∘ cos) ∘(tan ∘ cot)) === sin ∘ cos ∘ tan ∘ cot. It could also go to CompositionsBase.

jw3126/Setfield.jl#150

@pevnak
Copy link
Author

pevnak commented Nov 30, 2020

U know we have talked about it, I just did not know, if it fits there an where?

Should I move it?

@jw3126
Copy link
Member

jw3126 commented Nov 30, 2020

Not sure if you are aware. But ComposedOptic is just an alias for ComposedFunction.
https://github.com/JuliaObjects/Accessors.jl/blob/master/src/optics.jl#L90

So if you define decompose(f::ComposedFunction) = ... then composed lenses will automatically work.
So you can just put decompose(...) at the bottom of https://github.com/JuliaFunctional/CompositionsBase.jl/blob/master/src/CompositionsBase.jl

About normalize, let's wait if @tkf has an opinion on where it should live.

@jw3126
Copy link
Member

jw3126 commented Dec 6, 2020

@pevnak lets keep normalization here and move decompose to CompositionsBase.jl

propname(::PropertyLens{name})

returns the name of the propery lens
```julia
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```julia
```jldoctest```
Similar below.



"""
normalise(optic)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets spell it the American way normalize to be consistent with julia base.

@@ -275,3 +275,63 @@ function show_composition_order(io::IO, lens::ComposedOptic)
print(io, ")")
end


"""
decompose(optic)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be moved to CompositionsBase.jl

Copy link
Member

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks looks good overall, just some minor remarks. Also, we should add a test, that normalize yields the compiler preferred composition order (I think this might be violated with your current implementation):
https://github.com/JuliaObjects/Accessors.jl/blob/master/test/perf.jl

@jw3126
Copy link
Member

jw3126 commented Jul 5, 2021

@jw3126
Copy link
Member

jw3126 commented Jul 9, 2021

@pevnak no idea if this is still relevant for you, but I added decompose here JuliaFunctional/CompositionsBase.jl#9

@aplavin
Copy link
Member

aplavin commented Jan 4, 2024

Is any part of this PR relevant now that decompose has been in CompositionsBase for several years, @pevnak? The goal for normalize (another function introduced here) is not clear to me: the docstring seems to focus on a very specific case, not on general behavior.

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