Remove unreachable lines of sampling code

[MarcoGorelli]

I may be wrong here, but I think these lines are unreachable. I was trying to write a sensible test which covers them, but I think that might not be possible.

The function instantiate_steppers is only ever called from assign_step_methods.

In assign_step_methods, there is

    selected_steps = defaultdict(list)
    for var in model.free_RVs:
        if var not in assigned_vars:
            # determine if a gradient can be computed
            has_gradient = var.dtype not in discrete_types
            if has_gradient:
                try:
                    tg.grad(model.logpt, var)
                except (AttributeError, NotImplementedError, tg.NullTypeGradError):
                    has_gradient = False
            # select the best method
            selected = max(
                methods,
                key=lambda method, var=var, has_gradient=has_gradient: method._competence(
                    var, has_gradient
                ),
            )
            selected_steps[selected].append(var)

We have two cases:

• If model has no unnassigned free random variables, then selected_steps will be empty and so the loop for step_class, vars in selected_steps.items(): in instantiate_steppers will never be reached.

• if model has unnasigned free random variables, then selected_steps[selected] will be a list with var in it, and so if len(vars) == 0 will never be True.

In both cases, the continue in

        if len(vars) == 0:
            continue

won't be reached.

---

cc @michaelosthege - does this look right to you, or am I missing something?

[codecov]

Codecov Report

Merging #4261 (032b9e7) into master (a6295a3) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4261      +/-   ##
==========================================
- Coverage   88.15%   88.14%   -0.02%     
==========================================
  Files          87       87              
  Lines       14243    14242       -1     
==========================================
- Hits        12556    12553       -3     
- Misses       1687     1689       +2     

Impacted Files	Coverage Δ
pymc3/sampling.py	86.60% <100.00%> (+0.10%)	⬆️
pymc3/backends/report.py	90.90% <0.00%> (-2.10%)	⬇️

[michaelosthege]

The docstring should be improved and type annotations could be added:

steps : step function or vector of step functions
        One or more step functions that have been assigned to some subset of
        the model's parameters. Defaults to None (no assigned variables).

In contrast to what the docstring says, the code assumes that steps is a list and it can be empty.

This code should trigger line 138:

with pymc3.Model() as model:
    n = pymc3.Normal("n")
    u = pymc3.Normal("u")

    steppers = pymc3.sampling.instantiate_steppers(
        _model=model,
        steps=[
            pymc3.Slice([u])
        ],
        selected_steps={
            pymc3.Metropolis : [n],
            pymc3.NUTS : [],
        }
    )
    
    print(steppers)

Granted, it is not typically used that way, but instead of removing that len(vars) check, I would suggest to indent lines 134-138 and invert the condition.

[MarcoGorelli]

None has no .append method

[michaelosthege]

SHould be like this:

steps : list
        A list of zero or more step function instances that have been assigned to some subset of
        the model's parameters.

[MarcoGorelli]

Thanks for your review and help here, I've updated - for now I've just updated the docstring and inverted the condition, as I haven't figured out how to type some of the objects here yet (e.g. variables)

[michaelosthege]

And here, it should say

selected_steps : dict
    A dictionary that maps a step method class to a list of model variables.

[michaelosthege]

And here, it should say

selected_steps : dict
    A dictionary that maps a step method class to a list of model variables.

[MarcoGorelli]

Sure - I've added zero or more here because it's the fact that there could be zero here which was causing the continue line to be uncovered

[twiecki]

Thanks @MarcoGorelli and @michaelosthege.