Move to `Accessors.jl` from `Setfield.jl` #91

sunxd3 · 2024-03-09T17:47:35Z

Changes from `Setfield` to `Accessors`

Accessors is based on ComposeFunction, which means all the optics are either function or functor. Also, no get(obj, optic) interface, but optic(obj).
Lens abstract type doesn't exist in Accessors, IdentityLens is just identity, ComposedLens is now ComposedOptic (alias for ComposedFunction)
Inner and Outer are interchanged, l1 ∘ l2 in Setfield is l2 ∘ l1 in Accessors. Accessors adopted ⨟(\bbsemi "opposite compose") from CompositionBase: l1 ∘ l2 in Setfield is l1 ⨟ l2 in Accessors.

Unsure

The equivalence between VarNames are changed, before @varname(y[:], true) and @varname(y[1:100] (where y = zeros(10,10)) are considered to be ==. But with Accesssors, two IndexLens are generally not equivalent even when they have the same field because they are also functors.
Accessor.parse_obj_optic and Setfield.parse_obj_lens behave differently when deal with interpolation. I have tried and failed to maintain the current interface with Accessor.parse_obj_optic, so I am keeping Setfield.parse_obj_lens (and Setfield as a dependency), while replace the Lens with optic in the result of Setfield.parse_obj_lens.
Optics are functions or functors, how would this affect type stability?

codecov · 2024-03-10T06:39:42Z

Codecov Report

Attention: Patch coverage is 80.76923% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 82.72%. Comparing base (b342b3d) to head (22ad3fa).
Report is 6 commits behind head on main.

Files	Patch %	Lines
src/varname.jl	80.76%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   84.82%   82.72%   -2.11%     
==========================================
  Files           3        3              
  Lines         145      191      +46     
==========================================
+ Hits          123      158      +35     
- Misses         22       33      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

torfjelde

Good stuff @sunxd3 :)

The equivalence between VarNames are changed, before @varname(y[:], true) and @varname(y[1:100] (where y = zeros(10,10)) are considered to be ==. But with Accesssors, two IndexLens are generally not equivalent even when they have the same field because they are also functors.

I don't get this. Why does it matter that it's a functor?

From what I can tell it seems the issue is that @varname(y[1:100]) results in the indices field holding the range 1:100, not ConcretizedSlice(Base.OneTo(100)), which is what @varname(y[:], true) results in?

You're saying this was not the case before?

Optics are functions or functors, how would this affect type stability?

Maybe add some tests with @inferred?

Though for the subset of cases where care about, I'd be surprised if this would actually have any implications for type-stability.

The only worry is whether the fact that subtypes of Function will in certain cases not be specialized on, but Base.ComposedFunction does not subtype Function + from what I can tell, none of the optics subtype Function either, so we should be good.

torfjelde · 2024-03-11T13:43:42Z

src/varname.jl

-    is_static_lens(l::Lens)
-
-Return `true` if `l` does not require runtime information to be resolved.
+    is_static_optic(l)

-In particular it returns `false` for `Setfield.DynamicLens` and `Setfield.FunctionLens`.
+Return `true` if `l` is one or a composition of `identity`, `PropertyLens`, and `IndexLens`.
 """


Is there a reason why the docstring was changed here?

I subtly changed the function we used to have: is_static_lens returned false by default, now it is undefined by default.

Now the behavior is such that, when optic is one of identity, PropertyLens, and IndexLens, or composition of them, returns true; if optic is DynamicIndexLens or composition, it returns false, otherwise error.

So I thought being explicit is good here, but I should add something like for DynamicIndexLens, returns false.

src/varname.jl

torfjelde · 2024-03-11T13:52:44Z

src/varname.jl

+function Base.:∘(vn::VarName{sym,T}, optic) where {sym,T}
+    return VarName{sym}(getoptic(vn) ⨟ optic)
+end
+function Base.:∘(vn::VarName{sym,typeof(identity)}, optic) where {sym}
+    return VarName{sym}(optic)
 end


Hmm, I'm a bit uncertain about this.

We implemented this originally to replicate the composition behavior of Setfield.jl, but now this reversed and so it seems a bit strange to not also reverse this here?

This is one of the place I was uncertain too: I didn't want to break too many things, but maybe we should also adapt CompositionBase and introduce opcompose?

I propose we just remove this function, composition between a varname and a lens or optic is a bit strange for me anyway

The thing is, we are making VarName act as, what was before, a PropertyLens, in which case it does make sense to consider the composition IMO. We are still doing this, no?

sunxd3 · 2024-03-11T14:42:01Z

Before, Setfield has == defined for IndexLens (https://github.com/jw3126/Setfield.jl/blob/e31e1e361bd6a251124aa578122e6198b81197b5/src/lens.jl#L196), and we have AbstractPPL.ConcretizedSlice{Int, Base.OneTo{Int}}(Base.OneTo(100)) == UnitRange(1, 100), so the two VarName is equivalent. By saying "they are functor", I am just making a guess why Accessors doesn't have this == defined as Setfield does: it might be related to the semantic meaning of functions equivalence.

Co-authored-by: Tor Erlend Fjelde <[email protected]>

src/varname.jl

sunxd3 · 2024-03-19T15:49:54Z

@torfjelde another look?

Project.toml

sunxd3 · 2024-03-20T10:44:52Z

Just made some adjustment, Setfield is removed from the deps.

The interpolation is weaker compared to before. It is mainly attributed to Accessors.parse_obj_optic. Now more than one interpolated symbol (e.g. @varname($n1[$n2+1])), and interpolation of the LHS of a . expr (e.g. $name.a[1]) are disallowed.

torfjelde

The interpolation is weaker compared to before. It is mainly attributed to Accessors.parse_obj_optic. Now more than one interpolated symbol (e.g. @varname($n1[$n2+1])), and interpolation of the LHS of a . expr (e.g. $name.a[1]) are disallowed.

Woaah this seems not great; why do we feel the need to make this change?

EDIT: Maybe it doesn't matter that much. I was thinking it could cause issues because we do make use of interpolation and such in @submodel, buuut just realized this is a separate thing unrelated to VarName. So it's less of an issue than what I was imaging at first.

But I have actually used this myself (in the example discussed with Joel), in particular with the new Gibbs sampler where we will use @varname to specify what to sample rather than symbols. In such a scenario, it can be very convenient to do iteration + interpolation to construct the varnames you want (I used this feature myself when helping out Joel). So I'm still not super-keen on this change 😕

src/varname.jl

torfjelde · 2024-03-20T14:15:21Z

By saying "they are functor", I am just making a guess why Accessors doesn't have this == defined as Setfield does: it might be related to the semantic meaning of functions equivalence.

Might be worth raising an issue and asking about this, as IIUC it should be okay to define == for some of the optics, e.g the getindex one.

sunxd3 · 2024-03-20T14:20:49Z

The interpolation ability was added by @phipsgabler at jw3126/Setfield.jl#168, but this never got ported to Accessors.

I do think the interpolation can be quite handy, but that means we need to keep Setfield around for a while.

Co-authored-by: Tor Erlend Fjelde <[email protected]>

torfjelde · 2024-03-20T14:53:40Z

Is there a reason why we can't just copy-paste the macro? 🤷

yebai · 2024-03-21T14:42:47Z

Is there a reason why we can't just copy-paste the macro? 🤷

That should work since it is only a small utility function.

sunxd3 · 2024-03-22T14:57:59Z

@torfjelde @yebai function copied, not sure if it's small, but it should maintain the interpolation ability.

yebai

Thanks @sunxd3 -- good work!

torfjelde

Great stuff @sunxd3 :)

sunxd3 added 3 commits March 9, 2024 17:37

move to Accessors

bdb659e

fix tests

0cf9a27

fix test error

682212d

add compat

44f04a0

sunxd3 changed the title ~~[WIP] Move to Accessors.jl from Setfield.jl~~ Move to Accessors.jl from Setfield.jl Mar 10, 2024

version bump

205f506

sunxd3 requested review from torfjelde, devmotion and yebai March 11, 2024 04:48

torfjelde requested changes Mar 11, 2024

View reviewed changes

Update src/varname.jl

4ead3f1

Co-authored-by: Tor Erlend Fjelde <[email protected]>

sunxd3 commented Mar 11, 2024

View reviewed changes

src/varname.jl Outdated Show resolved Hide resolved

sunxd3 and others added 5 commits March 11, 2024 14:55

Update src/varname.jl

fa052ea

remove type piracy in show function

b710e8b

fix print behavior

056f142

removed composition of a varname to a lens

c4c4792

update doc

b9567dc

yebai reviewed Mar 19, 2024

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

Project.toml Outdated Show resolved Hide resolved

remove Setfield

d5dfff7

yebai approved these changes Mar 20, 2024

View reviewed changes

sunxd3 added 2 commits March 20, 2024 12:07

add some type stability tests and additional doctests

d229ad2

fix test error

680f32f

torfjelde requested changes Mar 20, 2024

View reviewed changes

src/varname.jl Outdated Show resolved Hide resolved

sunxd3 mentioned this pull request Mar 20, 2024

== not defined for IndexLens JuliaObjects/Accessors.jl#144

Open

Update src/varname.jl

bdee2fa

Co-authored-by: Tor Erlend Fjelde <[email protected]>

copy functions from Setfield and recover the interpolation abilities

103d005

fix some comments

22ad3fa

yebai approved these changes Mar 22, 2024

View reviewed changes

yebai requested a review from torfjelde March 28, 2024 12:14

torfjelde approved these changes Mar 28, 2024

View reviewed changes

sunxd3 merged commit f3fbce0 into main Mar 28, 2024
10 of 12 checks passed

sunxd3 deleted the sunxd/move_to_accessors branch March 28, 2024 15:25

This was referenced Apr 8, 2024

Stripe identity off ComposedOptic when test equality #94

Closed

Bring back ∘ function between varname and optic #95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to `Accessors.jl` from `Setfield.jl` #91

Move to `Accessors.jl` from `Setfield.jl` #91

sunxd3 commented Mar 9, 2024 •

edited by yebai

Loading

codecov bot commented Mar 10, 2024 •

edited

Loading

torfjelde left a comment •

edited

Loading

torfjelde Mar 11, 2024

sunxd3 Mar 11, 2024

torfjelde Mar 11, 2024

sunxd3 Mar 11, 2024

sunxd3 Mar 19, 2024

torfjelde Mar 20, 2024

sunxd3 commented Mar 11, 2024

sunxd3 commented Mar 19, 2024

sunxd3 commented Mar 20, 2024

torfjelde left a comment •

edited

Loading

torfjelde commented Mar 20, 2024

sunxd3 commented Mar 20, 2024

torfjelde commented Mar 20, 2024

yebai commented Mar 21, 2024

sunxd3 commented Mar 22, 2024

yebai left a comment

torfjelde left a comment

Move to Accessors.jl from Setfield.jl #91

Move to Accessors.jl from Setfield.jl #91

Conversation

sunxd3 commented Mar 9, 2024 • edited by yebai Loading

Changes from Setfield to Accessors

Unsure

codecov bot commented Mar 10, 2024 • edited Loading

Codecov Report

torfjelde left a comment • edited Loading

Choose a reason for hiding this comment

torfjelde Mar 11, 2024

Choose a reason for hiding this comment

sunxd3 Mar 11, 2024

Choose a reason for hiding this comment

torfjelde Mar 11, 2024

Choose a reason for hiding this comment

sunxd3 Mar 11, 2024

Choose a reason for hiding this comment

sunxd3 Mar 19, 2024

Choose a reason for hiding this comment

torfjelde Mar 20, 2024

Choose a reason for hiding this comment

sunxd3 commented Mar 11, 2024

sunxd3 commented Mar 19, 2024

sunxd3 commented Mar 20, 2024

torfjelde left a comment • edited Loading

Choose a reason for hiding this comment

torfjelde commented Mar 20, 2024

sunxd3 commented Mar 20, 2024

torfjelde commented Mar 20, 2024

yebai commented Mar 21, 2024

sunxd3 commented Mar 22, 2024

yebai left a comment

Choose a reason for hiding this comment

torfjelde left a comment

Choose a reason for hiding this comment

Move to `Accessors.jl` from `Setfield.jl` #91

Move to `Accessors.jl` from `Setfield.jl` #91

sunxd3 commented Mar 9, 2024 •

edited by yebai

Loading

Changes from `Setfield` to `Accessors`

codecov bot commented Mar 10, 2024 •

edited

Loading

torfjelde left a comment •

edited

Loading

torfjelde left a comment •

edited

Loading