TuringLang / AbstractPPL.jl

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

Provide symbol in `VarName` constructor as type parameter #17

Closed devmotion closed 3 years ago

devmotion commented 3 years ago

Constant propagation in the VarName does not work reliably as we noticed in https://github.com/TuringLang/DynamicPPL.jl/pull/221. Applying the changes in this PR improves performance noticeably.

IMO there is no reason to keep the existing type-unstable constructor. I deprecated it, therefore it is a non-breaking change.

codecov[bot] commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@b7a2e48). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #17   +/-   ##
=======================================
  Coverage        ?   82.45%           
=======================================
  Files           ?        1           
  Lines           ?       57           
  Branches        ?        0           
=======================================
  Hits            ?       47           
  Misses          ?       10           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b7a2e48...b315b92. Read the comment docs.

phipsgabler commented 3 years ago

I completely agree. This is basically the same effect as the recommendation to use Val{x} instead of Val(x), I believe.

marius311 commented 2 years ago

Following this deprecation, is there any succinct way to do VarName.((:x,:y,:z)) without the warning?

phipsgabler commented 2 years ago

Nothing shorter than map(n -> VarName{n}(), (:x, :y, :z)), I think. It's an inherently dynamic operation.

devmotion commented 2 years ago

I'm not a fan of eval usually but eval.(varname.((:x,:y,:z))) would work as well.

phipsgabler commented 2 years ago

I mean, if this were a frequent practical problem, I can imagine a solution similar to @gensym where

@varnames x y z[1]

expands (unhygienically) to

x = VarName{:x}()
y = VarName{:y}()
var"z[1]" = @varname(z[1])

But unless there's a pressing need, I'll leave it as an exercise for the interested reader.

marius311 commented 2 years ago

My use-case was within an inference library and I need to generate them programatically so the macro solution isn't what I want, but its an easy 1-line fix, I was just wondering if there did remain something builtin but its fine.