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

Rewrite #147

Closed
wants to merge 14 commits into from
Closed

Rewrite #147

wants to merge 14 commits into from

Conversation

jw3126
Copy link
Owner

@jw3126 jw3126 commented Jul 15, 2020

See #146 List of changes:

  • Get rid of Lens type, lenses are fully duck typed now
  • Replace get(obj, lens) by lens(obj)
  • Replace set(obj, lens, val) by set(lens, obj, val)
  • Replace modify(f, obj, lens) by modify(f, lens, obj)
  • Replace lens1 ∘ lens2 by lens2 ∘ lens1
  • Replace @set! by @reset

@tkf what you think about it. Especially replacing set(obj, lens, val) by set(lens, obj, val). In the function lens picture, this feels more natural. Also, it makes a possible future multi object modify(f, lens, obj, objs...) easier.

Tests pass locally, except for doctests. Docs are telling lies I plan to fix these when we know what API changes we actually want.
I agree this change is a good opportunity to change the package name.

@tkf
Copy link
Collaborator

tkf commented Jul 17, 2020

Wow! Thanks a lot for giving a shot at this!

Especially replacing set(obj, lens, val) by set(lens, obj, val). In the function lens picture, this feels more natural.

Isn't this convention usually for do block? Since we can't use anonymous function anyway, I'm not sure if it is a strong motivation on its own.

But maybe it'd let us add @set(...) do API like this?

obj = (a = (b = (c = (1, 2), d = 3,), e = 4,) f = 5)
otherthing = (something = obj, another = nothing)

new_obj = @set(otherthing.something, (40, 50, 20)) do obj
    a = obj.a
    b = a.b
    c = b.c
    return (b.e, obj.f, c[2])
end

(Or still with the function set but using something like IRTools.jl though that's rather mad. The idea is that get/set is pretty much like forward/backward passes of the reverse-mode auto-diff.)

Also, it makes a possible future multi object modify(f, lens, obj, objs...) easier.

I think one more nice thing about it is that we can get the curried version modify(f, lens) = (obj, objs...) -> modify(f, lens, obj, objs...) more consistently.

But, if we go with this PR, you can also do set(lens, obj, f(lens.((obj, objs...))...)), right? It's a bit tedious to write lens twice but I think it's nice overall since you can very clearly see that most of the fields in objs are not used this way. TBH, I'm not fully convinced that modify(f, lens, obj, objs...) is the right API. (That's why I created DataTools.modifying outside Setfield.jl. I wanted to play with it in data processing kind of contexts.)

Hmm... So set(lens, obj, val) is a tough one for me... I'm not super opinionated but I like that in the current set(obj, lens, val) the "flow" of the data is consistent (right-to-left, in some sense).

Let me know if you have other consideration on the new set API.

Replace @set! by @reset

This is nice! (So that we can reserve @set! for the in-place version.)

@jw3126
Copy link
Owner Author

jw3126 commented Jul 17, 2020

Thanks, you brought up a lot of interesting point. I will focus on set(lens, obj, val) vs set(obj, lens, val) and leave modify(f, ...) for later.

Hmm... So set(lens, obj, val) is a tough one for me... I'm not super opinionated but I like that in the current set(obj, lens, val) the "flow" of the data is consistent (right-to-left, in some sense).

Interestingly "getting the flow nice" was also for me the main reason to switch to set(lens, obj, val) order. My thinking was:

  • We humans have inconsistent preferences of preferred flow order. We like:
    • _.some + _.other = _.some.other = get_other(get_some(obj))
    • _[some] + _[other] = _[some][other] = get_other(get_some(obj))
    • some(_) + other(_) = some(other(_)) = get_some(get_other(obj))
  • So set(obj, lens, val) is a good fit for property + index flow order, while set(lens, obj, val) is better for function order
  • I think overall property and index lenses are used more then function lenses. So that is a strong motivation for the set(obj, lens, val) order.
  • However call syntax forces (lens, obj) order on get: lens(obj)
  • It would be a nice consistency if the order was the same for all three operations: get, set, modify. I prefer to be able to think in the function order picture all the time over having the property picture most of the time and the function picture sometimes.

Do you see any advantages of set(obj, lens, val) when it comes to extending syntax, e.g. like the partial evaluated modfiy you suggested?

@tkf
Copy link
Collaborator

tkf commented Jul 19, 2020

Thanks for explaining the reasoning behind set(lens, obj, val). Actually, my thought process was independent of the "internal" order of lens. That is to say, I liked that set(obj, lens, val) order has this "alignment"

obj′ = set(obj,     lens,   val)
@assert    obj′ >>′ lens == val

where >>′ is get in current Setfield and |> in this PR. The only alternative "flow-compatible" API is

obj′ = set(val,   lens,    obj)
@assert    val == lens <<′ obj′

where <<′(lens, obj) = obj >>′ lens. (But I think this method is very confusing.)

From this perspective, set(lens, obj, val) has this "spiral" flow:

      ,---.
      |   |
      v   |
set(lens, obj, val)
      |         ^
      |         |
      `---------+

That is to say, lens has to be in between obj and val. I think this is adequate description of lens; it describes a "relationship" of obj and val.

I guess another way to put it is that (1) set(obj, lens, val) is already compatible with function lens view if you consider it in the "|>-order" and (2) val is at the right location in this view.

@jw3126
Copy link
Owner Author

jw3126 commented Jul 19, 2020

Ok, thanks now I get it. I agree data flow compatibility is a nice property. I think with proper |> support in the macros that's the best solution.

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.

2 participants