AST Manipulation about dynamicppl.jl HOT 7 CLOSED

But I also agree that MLStyle has many more stuff included, which we don't need. Unfortunately, it seems to be the only thoroughly done and optimized pattern matcher.

Yes, that's basically my main objection. MLStyle seems cool but it's huge - and our expression matching and generation is very very simple in DynamicPPL. I agree though that of course we should strive for more readability and improving the code. In particular, I think we should rewrite build_model_input, just use ExprTools.jl instead of MacroTools to destructure the user-provided function, and then use a bunch of simple functions to parse the name of the arguments etc. BTW I think we should actually remove some of the parsing since it generates functions of the form f(T::Type{<:Float64}) instead of f(::Type{T}) where T<:Float64 as provided by the user - I don't see why we would want to do that, in particular when thinking about https://docs.julialang.org/en/v1/manual/performance-tips/#Be-aware-of-when-Julia-avoids-specializing-1.

And while we're at that, why not turn around macro hygiene and make expansion do it's work, instead of hand-crafting all those gensyms.

I played around with that and I think we should do that if possible. The main annoyance seemed to be that we have to escape all user provided code that is left untouched (it is easy to correct for everything else that we build in generate_dot_tilde and generate_tilde and in build_model_output).

from dynamicppl.jl.

I think where this type of pattern matching really shines is with more complex Expr and agree that it's a little overkill for getargs_dottilde. I'm not sure exactly what type of syntax/features you'd like to support and if MLStyle maybe to much. Perhaps we could talk more over email so I can understand more how I could contribute, I've shot @yebai an email to continue the discussion.

from dynamicppl.jl.

thanks @onetonfoot. Any PRs on improving readability and extensibility are welcome. If you'd like to contribute substantially to DynamicPPL, please feel free to drop me an email for a discussion.

from dynamicppl.jl.

I agree, I think we should get rid of the remaining parts of MacroTools in build_model_info (most/all other use cases should have been removed by the commits in the last two weeks). Any PR for that would be great!

However, IMO we should rather use simple expression manipulation (as illustrated in the current implementation of getargs_dottilde) than MLStyle. MLStyle is a quite heavy dependency (e.g., it depends on DataFrames) and IMO the resulting code does not increase readability (but of course that's subjective).

from dynamicppl.jl.

I do really like the match syntax of MLStyle and totally agree with @onetonfoot's readability argument -- currently, the upper parts of compiler.jl are still rather dense. And I kind of miss Haskell there.

But I also agree that MLStyle has many more stuff included, which we don't need. Unfortunately, it seems to be the only thoroughly done and optimized pattern matcher.

Maybe a good strategy (requiring some commitement, but not much more than a switch to MLStyle) would be to just start and try cleanly rewrite more parts of compiler.jl into a consistent style without any dependencies. There should be possibilities to factor out quite a lot of patterns, actually, and then we don't need a whole matcher package (after all, it's only 2 places where @match gets used in the fork!).

And while we're at that, why not turn around macro hygiene and make expansion do it's work, instead of hand-crafting all those gensyms.

from dynamicppl.jl.

I feel the current compiler implementation has been rewritten enough to make it nice and readable, with MacroTools alone. Should we close the issue?

from dynamicppl.jl.

I feel the current compiler implementation has been rewritten enough to make it nice and readable, with MacroTools alone.

Seconded - please feel free to re-open if more discussions are needed.

from dynamicppl.jl.