TuringLang / AbstractPPL.jl

Common types and interfaces for probabilistic programming
http://turinglang.org/AbstractPPL.jl/
MIT License
27 stars 7 forks source link

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

Closed sunxd3 closed 7 months ago

sunxd3 commented 8 months ago

Changes from Setfield to Accessors

Unsure

codecov[bot] commented 8 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sunxd3 commented 8 months ago

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.

sunxd3 commented 7 months ago

@torfjelde another look?

sunxd3 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

The interpolation ability was added by @phipsgabler at https://github.com/jw3126/Setfield.jl/pull/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.

torfjelde commented 7 months ago

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

yebai commented 7 months ago

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 commented 7 months ago

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