Skip to content

Add (i)adjoin#332

Merged
arybczak merged 2 commits intomasterfrom
add-adjoin
Aug 15, 2020
Merged

Add (i)adjoin#332
arybczak merged 2 commits intomasterfrom
add-adjoin

Conversation

@arybczak
Copy link
Copy Markdown
Collaborator

@arybczak arybczak commented Jul 31, 2020

Fixes #331.

It's reasonably fast when compared to a hand-written version (and obliterates lens):

benchmarking misc/adjoin/hand-written
time                 30.16 ns   (29.32 ns .. 31.73 ns)
                     0.991 R²   (0.978 R² .. 1.000 R²)
mean                 30.34 ns   (29.49 ns .. 32.66 ns)
std dev              2.938 ns   (876.6 ps .. 5.151 ns)
variance introduced by outliers: 90% (severely inflated)

benchmarking misc/adjoin/adjoin
time                 65.72 ns   (65.14 ns .. 66.25 ns)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 65.72 ns   (65.10 ns .. 66.22 ns)
std dev              1.131 ns   (853.5 ps .. 1.562 ns)
variance introduced by outliers: 21% (moderately inflated)

benchmarking misc/adjoin/adjoin/lens
time                 3.952 μs   (3.911 μs .. 4.019 μs)
                     0.997 R²   (0.991 R² .. 1.000 R²)
mean                 4.016 μs   (3.930 μs .. 4.307 μs)
std dev              309.4 ns   (65.82 ns .. 594.4 ns)
variance introduced by outliers: 78% (severely inflated)

When adjoining traversals that overlap I'm not sure which laws might be violated though. I suspect traversal composition law can be violated when the traversing function is stateful. I don't think setter laws can be violated.

Because of this I'm for exporting it normally.

@phadej
Copy link
Copy Markdown
Contributor

phadej commented Jul 31, 2020

The violated "law" is "5.5 No duplication of elements" in http://www.cs.ox.ac.uk/jeremy.gibbons/publications/iterator.pdf

@arybczak
Copy link
Copy Markdown
Collaborator Author

arybczak commented Aug 1, 2020 •
edited
Loading

@phadej I see. I don't get why is that a problem though.

λ> let once = simple
λ> let twice = simple `adjoin` simple
λ> over once (+1) 7
8
λ> over twice (+1) 7
8

Even though twice traverses the value twice, it changes it only once, because the second time it's still given the original value, not the one that was just modified (of course if you use traverseOf instead of over with a stateful traversing function you can most likely distinguish between them, but it's not obvious).

@adamgundry
Copy link
Copy Markdown
Member

adamgundry commented Aug 5, 2020

This looks great to me. I agree that exporting it normally is fine; it's no worse than say partsOf. I'd (still) suggest expanding on the docs slightly (#327 (comment)):

We should have "Monoid structure" sections of the Optics.[Ix](Setter|Traversal) docs, and perhaps an Optics.Monoidal module that documents all these monoids

although maybe a section in Optic would be preferable to a new module. I can draft it if you like.

@adamgundry adamgundry mentioned this pull request Aug 5, 2020
Open
@arybczak
Copy link
Copy Markdown
Collaborator Author

arybczak commented Aug 5, 2020

I can draft it if you like.

Yes please 🙏

arybczak
arybczak commented Aug 12, 2020
View reviewed changes
optics-core/src/Optics/IxTraversal.hs
--
-- >>> iview (ipartsOf (each `iadjoin` each)) ("x","y")
-- ([0,1,0,1],["x","y","x","y"])
-- >>> iset (ipartsOf (each `iadjoin` each)) (const ["a","b","c","d"]) ("x","y")
Copy link
Copy Markdown
Collaborator Author

@arybczak arybczak Aug 12, 2020

Choose a reason for hiding this comment

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

Nice example 👍

@arybczak
Copy link
Copy Markdown
Collaborator Author

arybczak commented Aug 12, 2020 •
edited
Loading

Thanks for the docs @adamgundry, LGTM to me :)

@arybczak arybczak merged commit 1e38abe into master Aug 15, 2020
@arybczak arybczak deleted the add-adjoin branch August 15, 2020 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add lensProduct and adjoin

3 participants

@arybczak @phadej @adamgundry