Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.67%. Comparing base (b96e368) to head (2925ee1).

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
- Coverage   82.14%   77.67%   -4.47%     
==========================================
  Files          30       30              
  Lines        4200     3919     -281     
==========================================
- Hits         3450     3044     -406     
- Misses        750      875     +125     

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

Pull Request Test Coverage Report for Build 11684382988

Details

• 0 of 10 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 82.182%

Totals
Change from base Build 11667882952:	-0.2%
Covered Lines:	3450
Relevant Lines:	4198

💛 - Coveralls

While I think you should feel free to merge this PR without this, could I ask whether you would consider exposing a function which returns the function + arguments that you pass to @code_warntype? i.e. something like

fargs, kwargs = get_args_for_analysis(model, varinfo, context))

with the same default values for varinfo and context as for model_warntype and model_typed?

I guess the implementation would be something like

function get_args_for_analysis(model, varinfo=default_thing, context=default_thing)
    args, kargs = DynamicPPL.make_evaluate_args_and_kwargs(model, varinfo, context)
    return (f, args...), kwargs
end

The reason to want this, is that

1. sometimes Base.code_ircode / Base.code_ircode_by_type is the thing you're after, rather than Base.code_typed (the former returns IRCode while the latter returned CodeInfo), and
2. if I want to inspect the IR generated by Mooncake.jl for a particular Turing.jl model (which I often want to do in order to debug stuff), I need to have access to the signature that I'm building the rule for, and I can get this from the arguments that you pass into @code_warntype.

Yep, can do @willtebbutt !

Wait; why is that function you defined simpler than just doing DynamicPPL.make_evaluate_args_and_kwargs(model, varinfo, context)?

Added optimize kwarg to the methods 👍

Wait; why is that function you defined simpler than just doing DynamicPPL.make_evaluate_args_and_kwargs(model, varinfo, context)?

It's a simpler interface, rather than implementation.

It's about ensuring that we have a single source of truth that you can point people towards (and put in the docs) that says "here is everything you need -- we promise that this is the thing that you should profile for performance", as opposed to having to know that the f field of model is the right function to look at (which I assume is an implementation detail rather than a guarantee that the interface makes), and that it must be combined with the output of make_evaluate_arg_and_kwargs.

edit: having now looked at this a bit more closely, am I correct that you can always do:

fargs, kwargs = DynamicPPL.make_evaluate_args_and_kwargs(model, varinfo, context)
fargs[1](fargs[2:end]...; kwargs...)

in order to evaluate the correct thing?

Also, I've just realised that this functionality is not tested, and doesn't appear in the docs. It would be good to add a unit test which just runs model_typed and model_warntype in order to check that they do indeed run.

Also, could you please add these to a debug section in the docs?

: having now looked at this a bit more closely, am I correct that you can always do:

Nah, it's

model.f(args...; kwargs...)

The evaluator f isn't returned by make_evaluator_args_and_kwargs. I guess returning the evaluator might be useful, but is it clear to have it as part of the "args" as you mentioned? Seems a bit confusing to me; why not return (f, args, kwargs) instead of (f, args...), kwargs?

Added docs and testing @willtebbutt

I guess returning the evaluator might be useful, but is it clear to have it as part of the "args" as you mentioned? Seems a bit confusing to me; why not return (f, args, kwargs) instead of (f, args...), kwargs?

I don't feel especially strongly either way. Personally, I like having it with the arguments because for Mooncake.jl-related stuff, where the distinction between function and argument is not really there (e.g. when using Base.code_ircode_by_type you just create the signature for the call). I can also see that most users probably do think about the function as being a separate thing from the args though, so I'm really very happy either way.