Test model logp before starting any MCMC chains

[StephenHogg]

This PR addresses #4116 - making find_MAP and sample check their starting conditions before running any chains. I probably need to work out what the linting settings this repo uses are because it seems like a fair bit of formatting has changed.

[michaelosthege]

I'm sorry, but can you do the PR without applying black first? I appreciate the intent & it's fine to do in the end, but at this point it makes pinpointing the relevant changes close to impossible..

[MarcoGorelli]

I'm sorry, but can you do the PR without applying black first? I appreciate the intent & it's fine to do in the end, but at this point it makes pinpointing the relevant changes close to impossible..

I think the opposite has happened - black has already been applied to the entire codebase, it looks like here they've reverted some of its changes - e.g., black wouldn't do this:

+ from .util import (chains_and_samples, dataset_to_point_dict, get_default_varnames, get_untransformed_name,
+                    is_transformed_name, update_start_vals)

@StephenHogg please see the Python Style guide for this repo

[StephenHogg]

Sorry about this - had auto-linting on in my GUI and didn't realise. Have a look now, hopefully it's clearer.

[michaelosthege]

If I remember correctly, the downstream code treats a list of start as "start points for each chain", which could explain your index error.

[StephenHogg]

yeah - the start points object doesn't get used further down, it's just for checking the initial conditions in a way that makes handling both the case when it's a dictionary and the case when it's an array easy. `Start` is still what gets fed into functions later on, hence my confusion haha

…

On Tue, Nov 10, 2020, 19:12 Michael Osthege ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pymc3/sampling.py <#4211 (comment)>: > @@ -419,6 +419,29 @@ def sample( """ model = modelcontext(model) + if start is None: + start = model.test_point + else: + if isinstance(start, dict): + update_start_vals(start, model.test_point, model) + else: + for chain_start_vals in start: + update_start_vals(chain_start_vals, model.test_point, model) + + start_points = [start] if isinstance(start, dict) else start If I remember correctly, the downstream code treats a list of start as "start points for each chain", which could explain your index error. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4211 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADFQOPVG3DUPEL5A43RSBIDSPDYYBANCNFSM4TPH3KGA> .

[michaelosthege]

The error you posted in #4116 could also be a cause of invalid model test points. Could be that not all distributions have tests points.
Try to copy the model from the failing test case into a notebook to inspect it.

[StephenHogg]

Just for clarity - what's the path forward here? Sorry for the bother

[ColCarroll]

I think running this check is generally a good idea, but I think it needs to be put in at the right place. If you look at sampling.py, this change will get rid of a bunch of downstream code (there are some if start is None, but I don't think in a great way.

What if we had a _check_start_point function, and it got called at the end of init_nuts? I think it would contain model.check_test_point, and the nice error messages, but it would not do anything to the start argument passed to it.

[ColCarroll]

Sorry, I just read the attached issue, and it seems like that was steering @StephenHogg to put the changes where they are. Interested to hear if what you and @michaelosthege think!

[StephenHogg]

As a first time contributor I defer to Michael! :)

[michaelosthege]

As a first time contributor I defer to Michael! :)

I think Colin is right: The block could easily become its own function. That also makes it easier to test, or improve.

[StephenHogg]

Ok - are you also saying the new function should be called at the end of init_nuts instead of where it is now?

[michaelosthege]

Ok - are you also saying the new function should be called at the end of init_nuts instead of where it is now?

No, the NUTS initialization often suffers from inf/NaN and I think it's more useful to check that before initialization.

[ColCarroll]

The "main path" logic right now is:

• start goes into the function
• init_nuts is called, and comes up with its own start_
  • usually this is by taking the test point, and adding Uniform(-1, 1) noise
  • start_ in init_nuts is used to initialize a mean for a running variance estimate
• start_ is overwritten in the main loop whenever start is not None

I think Michael's right that start_ also needs to be checked, since it is secretly being used internally (which is bad, but shouldn't be fixed here). But looking at the code, I think it can be done right after returning from init_nuts, otherwise there would be a ton of checks.

Concretely, I'm suggesting

• a function that checks a user-supplied start value very early (around line 460, where there's already a check on start)
• again checking start_ after it returns from init_nuts (around line 490)
• perhaps in a followup PR, having the function check other initialization schemes (there's an else branch for discrete models I've been ignoring...)

[StephenHogg]

@michaelosthege any more thoughts on the above? Would like to make sure I'm clear about what I'm coding up before starting again

[michaelosthege]

@StephenHogg listen to Colin on this one. He's much more literate in what the NUTS code is actually doing. With those checks in their own function, you can run them before & after NUTS initialization.
It costs a few model evaluations, but hey, this is MCMC sampling and it gives us more interpretable errors.

[StephenHogg]

I've shifted this into a function called pm.util.check_start_vals. In the process, I've spotted (and fixed) a bug in model.check_test_point. The function's test_point argument was being ignored at all times.

[StephenHogg]

Looking at the test output, it seems like a few other tests (e.g. test_nuts_error_reporting in test_hmc.py) are actually broken by the changes in this branch. Any guidance as to how to handle this appreciated.

[StephenHogg]

Here's the output I get from pytest at this point, if that helps. Some of these are a bit mystifying, as I'm not sure why I'd be getting a max recursion depth error on a test that I've not touched, for instance. Will push one more change to format the error string a bit more nicely, but after that I think I'm probably stuck for now.

[ColCarroll]

this looks nice! I took a look at most of the the test failures, and they're surprisingly helpful. Feel free to ping again if you need more help, but I think this is close:

• Delete test_hmc.py:test_nuts_error_reporting. your check is a better one for the same behavior.
• test_sampling.py:test_deterministic_of_observed looks like a flake. let's ignore that and hope it goes away. if it doesn't, make the rtol bigger.
• test_examples.py::TestLatentOccupancy::test_run is interesting, and looks like a legit failure you found! In this case, the likelihood is passing parameters in the wrong order. It should be
  pm.ZeroInflatedPoisson("y", psi, theta, observed=y) (note that psi and theta are switched). I imagine it was passing because the multipart sampling got everything to a reasonable place.
• Two failures in pymc3/tests/test_step.py can also be either deleted, or ported to the new exception you throw -- it looks like we have a SamplingError defined, which may be a good, specific error to raise instead of a ValueError.

[StephenHogg]

The only thing still failing at this point is one test in test_step.py, which is because it is expecting a ParallelSamplingError but the new check is just returning a SamplingError. I'm happy to shift this over, but wanted to check I'm not violating the spirit of this test?

Edit: the flaky test is also not passing, but that definitely passes locally

[StephenHogg]

Hi @ColCarroll - the only thing that still fails now is test_sampling.py:test_deterministic_of_observed when FLOATX is set to float32 (doesn't appear to be a problem with 64 bit floats?). I can increase the rtol here but it's already 1e-3, got any preference?

[ColCarroll]

This looks great! What if you loosen the tolerances on the test, but also open a bug and mention that it got worse when this PR was merged? That's very strange...

I think the last two things are:

• Remove draft status of this pr
• Add a line to RELEASE_NOTES.md

[codecov]

Codecov Report

Merging #4211 (ddf9fc3) into master (2723b6c) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4211      +/-   ##
==========================================
- Coverage   88.14%   87.95%   -0.19%     
==========================================
  Files          87       87              
  Lines       14243    14248       +5     
==========================================
- Hits        12554    12532      -22     
- Misses       1689     1716      +27     

Impacted Files	Coverage Δ
pymc3/model.py	89.05% <ø> (-0.12%)	⬇️
pymc3/sampling.py	85.39% <100.00%> (-1.10%)	⬇️
pymc3/tuning/starting.py	83.72% <100.00%> (+1.90%)	⬆️
pymc3/step_methods/hmc/base_hmc.py	90.83% <0.00%> (-7.50%)	⬇️
pymc3/backends/report.py	90.90% <0.00%> (-2.10%)	⬇️
pymc3/parallel_sampling.py	86.79% <0.00%> (-1.58%)	⬇️
pymc3/step_methods/hmc/quadpotential.py	79.03% <0.00%> (-0.54%)	⬇️

[twiecki]

Seems like there are still conflicts though:

[StephenHogg]

Seems like there are still conflicts though:

Yes, that's what I'm saying - I can either leave the conflict in, in which case I can't merge, or I can resolve the conflict in which case linting fails because there's an unneeded import. It's a Catch-22.

[MarcoGorelli]

Seems like there are still conflicts though:

Yes, that's what I'm saying - I can either leave the conflict in, in which case I can't merge, or I can resolve the conflict in which case linting fails because there's an unneeded import. It's a Catch-22.

Shouldn't be a catch-22 😄

Can you try

git fetch --all --prune
git merge upstream/master

Then, in pymc3/util.py, you'll see something like

<<<<<<< HEAD
=======
from numpy import ndarray
>>>>>>> upstream/master

Change it to

(i.e., choose the current changes, ignore the incoming ones)

Then,

git add -u
git commit
git push -u origin HEAD

---

for more on git, I heartily recommend the pro git book

[StephenHogg]

As before - there's a mysterious new test failure

[ColCarroll]

Wow, CI got changed under you!

test_examples.py::TestLatentOccupancy::test_run got reverted during your merge, I think: pm.ZeroInflatedPoisson("y", psi, theta, observed=y) should be pm.ZeroInflatedPoisson("y", psi, theta, observed=y)

[StephenHogg]

This new error doesn't seem to have much to do with the code I wrote? Not sure, though

[MarcoGorelli]

This new error doesn't seem to have much to do with the code I wrote? Not sure, though

can you check if that test passes when you run it locally?

pytest pymc3/tests/test_step.py::TestMLDA::test_acceptance_rate_against_coarseness

[StephenHogg]

@MarcoGorelli passes locally, had to update theano-pymc to version 1.0.11 to do it. Here's the output:

(pymc3) shogg@192:~/git/pymc3$ pytest pymc3/tests/test_step.py::TestMLDA::test_acceptance_rate_against_coarseness
================================================================================================================================== test session starts ==================================================================================================================================
platform darwin -- Python 3.7.5, pytest-5.0.1, py-1.9.0, pluggy-0.13.1
rootdir: /Users/shogg/git/pymc3, inifile: setup.cfg
collected 1 item                                                                                                                                                                                                                                                                        

pymc3/tests/test_step.py .                                                                                                                                                                                                                                                        [100%]

=================================================================================================================================== warnings summary ====================================================================================================================================
pymc3/tests/test_step.py::TestMLDA::test_acceptance_rate_against_coarseness
pymc3/tests/test_step.py::TestMLDA::test_acceptance_rate_against_coarseness
pymc3/tests/test_step.py::TestMLDA::test_acceptance_rate_against_coarseness
  /Users/shogg/git/pymc3/pymc3/step_methods/mlda.py:383: UserWarning: The MLDA implementation in PyMC3 is still immature. You should be particularly critical of its results.
    "The MLDA implementation in PyMC3 is still immature. You should be particularly critical of its results."

-- Docs: https://docs.pytest.org/en/latest/warnings.html
========================================================================================================================= 1 passed, 3 warnings in 13.26 seconds =========================================================================================================================

[StephenHogg]

Wait, all checks have passed now? Maybe the test was flaky?

[twiecki]

Yes, I think so. Thanks @StephenHogg!

[StephenHogg]

Whew, thanks

[ColCarroll]

+1 Thanks for sticking with us, @StephenHogg -- this was trickier than expected, but I think it will really improve lots of people's experiences.