Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
1e267f8
allowed use of Setfield.lens in VarName
torfjelde May 18, 2021
286e211
implemented subsumes for lenses
torfjelde May 18, 2021
eb65b76
varname macro now uses lenses by default
torfjelde May 18, 2021
e8058a4
Merge branch 'main' into tor/lenses
torfjelde Jul 26, 2021
b72243a
added concretize method for instantiating a variable on a particular …
torfjelde Jul 27, 2021
122092d
added some type-piracy because its a free world
torfjelde Jul 27, 2021
29e051b
fixed and added doctests
torfjelde Jul 27, 2021
6092b7c
fixed varname
torfjelde Jul 29, 2021
2259b30
fixed varname doctest
torfjelde Jul 29, 2021
298e1fc
allow specifying whether we want to concretize or not in varname
torfjelde Jul 29, 2021
9dd6cf7
need to escape indices
torfjelde Jul 29, 2021
b94bcce
fixed show
torfjelde Jul 29, 2021
9af3694
fix the subsume behavior
torfjelde Jul 29, 2021
e26307f
fixed doctests
torfjelde Jul 29, 2021
fff6f1f
made the construction of the lens for varname a bit nicer
torfjelde Jul 29, 2021
e47abbb
fixed vinds
torfjelde Jul 29, 2021
5328a6b
removed duplicate method
torfjelde Jul 29, 2021
924f45d
make concretize an argument instead of kwarg
torfjelde Jul 29, 2021
79118eb
make concretize an argument instead of kwarg
torfjelde Jul 29, 2021
71af2e4
fixed subsume for lenses
torfjelde Jul 29, 2021
e2ffdad
use using instead of import
torfjelde Jul 29, 2021
638bafc
extend composition rules to varname
torfjelde Jul 29, 2021
b4db7a4
use Setfield.parse_obj_lens and allow begin
torfjelde Jul 31, 2021
8299329
added proper fix for begin in indexing
torfjelde Jul 31, 2021
33d6777
removed a wild StatsBase that had appeared
torfjelde Jul 31, 2021
38ac60a
removed now redundant replace_basesysm
torfjelde Jul 31, 2021
e14ce7c
dont export nonexistent methods
torfjelde Jul 31, 2021
854bbe1
dont allow Tuple in VarName anymore but only allow Lens
torfjelde Jul 31, 2021
f010a1b
sort of fixed _issubrange
torfjelde Jul 31, 2021
253dffe
fixed a doctests
torfjelde Jul 31, 2021
d31461e
added get and set for VarName
torfjelde Aug 1, 2021
b596c11
added backwards compat constructor for VarName
torfjelde Aug 1, 2021
c72faab
added some weird behavior from existing codebase...
torfjelde Aug 1, 2021
fc53f29
no longer need special handling of begin after Setfield 0.7.1
torfjelde Aug 1, 2021
111f805
remove show for old tuple-based VarName
torfjelde Aug 1, 2021
84badce
forgot to check if concretize was true in varname
torfjelde Aug 1, 2021
8c1d193
renamed tuple2indexlens
torfjelde Aug 1, 2021
3a854e2
formatting
torfjelde Aug 1, 2021
2019180
disallow usage of dynamic lenses in VarName
torfjelde Aug 1, 2021
a397bfe
formatting and new show
torfjelde Aug 1, 2021
5905206
updated comment
torfjelde Aug 1, 2021
3f287f5
fixed doctests
torfjelde Aug 17, 2021
b616902
Update Project.toml
yebai Aug 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ version = "0.2.0"

[deps]
AbstractMCMC = "80f14c24-f653-4e6a-9b94-39d6b0f70001"
Setfield = "efcf1570-3423-57d1-acb7-fd33fddbac46"
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"

[compat]
AbstractMCMC = "2, 3"
Expand Down
107 changes: 95 additions & 12 deletions src/varname.jl
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
using Setfield
import Setfield: PropertyLens, ComposedLens, IdentityLens, IndexLens, DynamicIndexLens

"""
VarName{sym}(indexing::Tuple=())

Expand Down Expand Up @@ -26,10 +29,10 @@ julia> @varname x[:, 1][1+1]
x[:,1][2]
```
"""
struct VarName{sym, T<:Tuple}
struct VarName{sym, T}
indexing::T
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we rename indexing to lens @phipsgabler ?

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to suggest that already, but expected someone to complain about incompatibility! I'm all for it.


VarName{sym}(indexing::Tuple=()) where {sym} = new{sym,typeof(indexing)}(indexing)
VarName{sym}(indexing=()) where {sym} = new{sym,typeof(indexing)}(indexing)
end

"""
Expand All @@ -45,7 +48,7 @@ julia> VarName(@varname(x[1][2:3]))
x
```
"""
function VarName(vn::VarName, indexing::Tuple = ())
function VarName(vn::VarName, indexing = ())
return VarName{getsym(vn)}(indexing)
end

Expand Down Expand Up @@ -89,7 +92,7 @@ getindexing(vn::VarName) = vn.indexing
Base.hash(vn::VarName, h::UInt) = hash((getsym(vn), getindexing(vn)), h)
Base.:(==)(x::VarName, y::VarName) = getsym(x) == getsym(y) && getindexing(x) == getindexing(y)
Copy link
Member

Choose a reason for hiding this comment

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

We need to be careful here now... what about DynamicIndexLens, containing a closure?

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case they won't be equal, right? Which IMO seems like sensible behavior, no? They're not the same but they have the same behavior.

Seems like the only other alternative is to completely drop the definition of == which also seems a bit much?

Copy link
Member

Choose a reason for hiding this comment

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

Agree to both, this was more like... warn the callers of that, and check that we ourselves don't fall over this (which should be easy, though, as we are only concerned with concretized varnames anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure 👍 How should do this though? Warning will be slow. Error is too much. Docstring?


function Base.show(io::IO, vn::VarName)
function Base.show(io::IO, vn::VarName{<:Any, <:Tuple})
print(io, getsym(vn))
for indices in getindexing(vn)
print(io, "[")
Expand All @@ -98,6 +101,15 @@ function Base.show(io::IO, vn::VarName)
end
end

function Base.show(io::IO, vn::VarName{<:Any, <:Lens})
Copy link
Member

Choose a reason for hiding this comment

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

There's something off with DynamicIndexLens:

julia> @varname x[1][end]
x[1]Setfield.DynamicIndexLens{var"#3#4"}(var"#3#4"())(_)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's just my weird show impl below since Setfield.jl doesn't have a print_application (which is what we want here) for DynamicIndexLens. I didn't give it too much thought since we're not going to be using DynamicLens in DPPL anyways, i.e. we will always concretize.

print(io, getsym(vn))
Setfield.print_application(io, vn.indexing)
end

# TODO: Should this really go here?
Copy link
Member Author

Choose a reason for hiding this comment

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

The answer is no of course, but I'm not bothered to start impl this in a nicer way until we've decided that this is a path we want to take.

Setfield.print_application(io::IO, l::IndexLens) = print(io, "[", join(map(prettify_index, l.indices), ", "), "]")
Setfield.print_application(io::IO, l::DynamicIndexLens) = print(io, l, "(_)")

prettify_index(x) = string(x)
prettify_index(::Colon) = ":"

Expand Down Expand Up @@ -214,7 +226,65 @@ _issubrange(i::ConcreteIndex, j::ConcreteIndex) = issubset(i, j)
_issubrange(i::Union{ConcreteIndex, Colon}, j::Colon) = true
_issubrange(i::Colon, j::ConcreteIndex) = true

# Idea behind `subsumes` for `Lens` is that we traverse the two lenses in parallel,
# checking `subsumes` for every level. This for example means that if we are comparing
# `PropertyLens{:a}` and `PropertyLens{:b}` we immediately know that they do not subsume
# each other since at the same level/depth they access different properties.
subsumes(t::ComposedLens, u::ComposedLens) = subsumes(t.outer, u.outer) && subsumes(t.inner, u.inner)

# If `t` is still a composed lens, then there is no way it can subsume `u` since `u` is a
# leaf of the "lens-tree".
subsumes(t::ComposedLens, u::PropertyLens) = false
# Here we need to check if `u.outer` (i.e. the next lens to be applied from `u`) is
# subsumed by `t`, since this would mean that the rest of the composition is also subsumed
# by `t`.
subsumes(t::PropertyLens, u::ComposedLens) = subsumes(t, u.outer)

# For `PropertyLens` either they have the same `name` and thus they are indeed the same.
subsumes(t::PropertyLens{name}, u::PropertyLens{name}) where {name} = true
# Otherwise they represent different properties, and thus are not the same.
subsumes(t::PropertyLens, u::PropertyLens) = false

# Indices subsumes if they are subindices, i.e. we just call `_issubindex`.
# FIXME: Does not support `DynamicIndexLens`.
# FIXME: Does not correctly handle cases such as `subsumes(x, x[:])`
# (but neither did old implementation).
subsumes(t::IndexLens, u::IndexLens) = _issubindex(t.indices, u.indices)

"""
concretize(l::Lens, x)

Return `l` instantiated on `x`, i.e. any runtime information evaluated using `x`.
"""

Copy link
Member

Choose a reason for hiding this comment

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

This empty line seems wrong, I don't think the docstring will show up anywhere.

concretize(I::Lens, x) = I
concretize(I::DynamicIndexLens, x) = IndexLens(I.f(x))
function concretize(I::ComposedLens, x)
x_inner = get(x, I.outer)
return ComposedLens(concretize(I.outer, x), concretize(I.inner, x_inner))
end
"""
concretize(vn::VarName, x)

Return `vn` instantiated on `x`, i.e. any runtime information evaluated using `x`.

# Examples
```jldoctest
julia> x = (a = [1.0 2.0;], );

julia> vn = @varname(x.a[1, :])
x.a[1, :]

julia> AbstractPPL.concretize(vn, x)
x.a[1, :]

julia> vn = @varname(x.a[1, end][:]);

julia> AbstractPPL.concretize(vn, x)
x.a[1, 2][:]
```
"""
concretize(vn::VarName, x) = VarName(vn, concretize(vn.indexing, x))

"""
@varname(expr)
Expand All @@ -231,30 +301,43 @@ julia> @varname(x).indexing
()

julia> @varname(x[1]).indexing
((1,),)
(@lens _[1])

julia> @varname(x[:, 1]).indexing
((Colon(), 1),)
(@lens _[:, 1])

julia> @varname(x[:, 1][2]).indexing
((Colon(), 1), (2,))
(@lens _[:, 1][2])

julia> @varname(x[1,2][1+5][45][3]).indexing
((1, 2), (6,), (45,), (3,))
(@lens _[1, 2][6][45][3])
```

!!! compat "Julia 1.5"
Using `begin` in an indexing expression to refer to the first index requires at least
Julia 1.5.
"""
macro varname(expr::Union{Expr, Symbol})
return esc(varname(expr))
return varname(expr)
end

varname(sym::Symbol) = :($(AbstractPPL.VarName){$(QuoteNode(sym))}())
function varname(expr::Expr)
if Meta.isexpr(expr, :ref)
sym, inds = vsym(expr), vinds(expr)
if Meta.isexpr(expr, :ref) || Meta.isexpr(expr, :.)
sym = vsym(expr)

# Need to recursively unwrap until we reach the outer-most variable.
# TODO: implement as recursion?
curexpr = expr
while !(curexpr.args[1] isa Symbol)
curexpr = curexpr.args[1]
end

# Then we replace the variable with `_`, to get an expression we can
# use `lensmacro` on.
curexpr.args[1] = :_
inds = Setfield.lensmacro(identity, expr)

return :($(AbstractPPL.VarName){$(QuoteNode(sym))}($inds))
else
error("Malformed variable name $(expr)!")
Expand Down Expand Up @@ -294,7 +377,7 @@ function vsym end

vsym(expr::Symbol) = expr
function vsym(expr::Expr)
if Meta.isexpr(expr, :ref)
if Meta.isexpr(expr, :ref) || Meta.isexpr(expr, :.)
Copy link
Member

@phipsgabler phipsgabler Jul 29, 2021

Choose a reason for hiding this comment

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

We might also need to handle GlobalRef here (and similarly, up in varname).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the more stable option is to use parse_obj_lens first to split the vsym and the indexing, and then just operate on the constructed VarName? Now that the vinds is finally going to be removed from the tilde functions, is there any more need for vsym/vinds on their own?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with the usage of parse_obj_lens; that will simplify things:)

Regarding vsym/vinds, I don't know 😕 IMO vsym can be nice still, as it's just a nice way of extracting the symbol used as a reference, but vinds I agree is useless now.

return vsym(expr.args[1])
else
error("Malformed variable name $(expr)!")
Expand Down