Remove necessity to define likelihood variable inside model, is done … #2339

hvasbath · 2017-06-21T11:15:25Z

…now inside sampler init

added testing intermediate stage loading results
renamed ATMIP_sample to smc_sample
minor fix in clearing existing stages

Thumbs up to @junpenglao 's idea!

…now inside sampler init * added testing intermediate stage loading results * renamed ATMIP_sample to smc_sample * minor fix in clearing existing stages

junpenglao · 2017-06-21T11:22:32Z

pymc3/step_methods/smc.py

@@ -147,6 +147,10 @@ def __init__(self, vars=None, out_vars=None, n_chains=100, scaling=1., covarianc
        vars = inputvars(vars)

        if out_vars is None:
+            if not any(likelihood_name == RV.name for RV in model.unobserved_RVs):


I think we should not leave the option for user to define Deterministic likelihood.

What is a deterministic likelihood?

this is basically to check if by any chance the likelihood name variable is defined already, repeated initialisation of the same model would also add the Deterministic again into the model, which would result in an error
this is basically a workaround to get the likelihood written into the trace @fonnesbeck

Couldn't you add the logp as a sampler statistic? That it would be attached to the trace but didn't clutter the normal variables.

how would I do that? @aseyboldt

Each step method can set generates_stats to True and define dtypes for the individual values in stats_dtypes. You can have a look at nuts.py lines 75 to 87. The step method must then return for each step a tuple (q, [stats]), where q is the sampled point and stats contains the statistics it wants to store.

Ah that feature didnt exist when I started writing SMC, I guess now it could be even possible to get it into the common pm.sample API ... however, that would require some major changes I have no time to do that in the near future

@aseyboldt I didnt know about that as well!

fonnesbeck · 2017-06-21T11:31:33Z

pymc3/step_methods/smc.py

@@ -28,7 +28,7 @@
 from .arraystep import metrop_select
 from ..backends import smc_text as atext

-__all__ = ['SMC', 'ATMIP_sample']
+__all__ = ['SMC', 'smc_sample']


Keep acronyms in all caps, so SMC_sample

ok! will change it

The other dedicated sampling functions are named the other way round: sample_gp, sample_ppc.

And according to pep8 acronyms in class names should only have the first letter capitalized: http://legacy.python.org/dev/peps/pep-0008/#descriptive-naming-styles
I guess that means that acronyms in function names should be all lower case (which is the case in the newer python stdlib modules I think, eg ipaddress).

+1 on sample_SMC().

@twiecki Sure? Shouldn't it be sample_smc? I think this goes clearly against the common convention.

yeah, I prefer sample_smc, too. sample_SMC looks clunky and doesn't follow convention.

fonnesbeck · 2017-06-21T11:32:11Z

pymc3/step_methods/smc.py

@@ -419,9 +423,9 @@ def resample(self):
        return outindx


-def ATMIP_sample(n_steps, step=None, start=None, homepath=None, chain=0, stage=0, n_jobs=1,
+def smc_sample(n_steps, step=None, start=None, homepath=None, chain=0, stage=0, n_jobs=1,


SMC_sample

…ture test

hvasbath · 2017-06-21T15:04:36Z

I cant see why the tests are marked as failed? There are no failed tests in the list!

fonnesbeck · 2017-06-21T15:09:09Z

PyLint failures?

junpenglao · 2017-06-21T15:13:01Z

It happened in #1977 as well. Is it relate to xfail?

aseyboldt · 2017-06-21T15:38:16Z

@hvasbath Doesn't this do an eigenvalue decomposition of the same proposal correlation matrix in each step right now? There are already normal proposal classes that do a cholesky decomposition once and use that to draw samples in metropolis.py. That should be much faster.

hvasbath · 2017-06-21T16:01:39Z

@aseyboldt No, it updates the proposal covariance once each transition state, then uses the same proposal covariance throughout the sampling steps. The proposal_steps are created only once at the beginning of each chain for the whole chain. So no repeated RNG call here. Calling the RNG each step is unbelievably inefficient as discussed here: #1034

aseyboldt · 2017-06-21T16:40:30Z

On first glance that looks like it might just be the eigen decomposition I was talking about. To draw a mvnormal you need to factor the covariance matrix first. This is very expensive (O(n^3)), so you only want to do that once. After that you only need to multiply independent normal draws with the factored matrix, which is much faster than the factorization.

In [30]: cov = np.random.randn(500, 500)
In [31]: cov = 500 * np.eye(500) + cov @ cov.T
In [32]: prop = pm.step_methods.metropolis.MultivariateNormalProposal(cov)
In [33]: %timeit prop()
81.6 µs ± 902 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
In [34]: prop = pm.step_methods.smc.MultivariateNormalProposal(cov)
In [35]: %timeit prop()
140 ms ± 665 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

You would still get a bit of a speedup by drawing several samples at the same time I guess...

hvasbath · 2017-06-21T17:56:28Z

Ah now I get it. Good point! Also did miss that there was improvement in the metropolis MVNormal -however we should improve that to be able to draw several values at once ...

…used import

ColCarroll · 2017-06-22T03:59:57Z

I restarted that build -- this is an ongoing issue that

pymc3/tests/test_examples.py::TestARM5_4::test_run

takes O(10 minutes) to run. Travis kills the job if there's no output for 10 minutes. A partial fix would be turning on the progressbar to get some output (a real fix would be making the test faster, or adding a docstring explaining why it takes 10 minutes!)

twiecki · 2017-06-23T10:08:33Z

pymc3/step_methods/smc.py

@@ -419,9 +408,9 @@ def resample(self):
        return outindx


-def ATMIP_sample(n_steps, step=None, start=None, homepath=None, chain=0, stage=0, n_jobs=1,
+def sample_SMC(n_steps, step=None, start=None, homepath=None, chain=0, stage=0, n_jobs=1,


sample_smc

before I change it again could we please vote ;) or is this the final statement? because I had it already once like that ;) ...

We're at 2 vs 1 currently (@aseyboldt, @twiecki and @fonnesbeck). @junpenglao @ColCarroll?

ColCarroll · 2017-06-23T10:43:09Z

I vote lower case!

…

On Fri, Jun 23, 2017, 6:41 AM Thomas Wiecki ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pymc3/step_methods/smc.py <#2339 (comment)>: > @@ -419,9 +408,9 @@ def resample(self): return outindx -def ATMIP_sample(n_steps, step=None, start=None, homepath=None, chain=0, stage=0, n_jobs=1, +def sample_SMC(n_steps, step=None, start=None, homepath=None, chain=0, stage=0, n_jobs=1, We're at 2 vs 1 currently ***@***.*** <https://github.com/aseyboldt>, @twiecki <https://github.com/twiecki> and @fonnesbeck <https://github.com/fonnesbeck>). @junpenglao <https://github.com/junpenglao> @ColCarroll <https://github.com/colcarroll>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACMHELW0JrARO-JDWZiEPgZ_VxWamjy4ks5sG5ZFgaJpZM4OAz2K> .

junpenglao · 2017-06-23T10:55:33Z

lower case, i agree with.

fonnesbeck · 2017-06-23T12:27:58Z

I vote that we just be consistent. What is the policy? Lower case for functions and caps for classes?

twiecki · 2017-06-23T12:30:39Z

I like that policy.

fonnesbeck · 2017-06-23T12:39:35Z

Sounds good. Go ahead and change it back @hvasbath. Sorry for the hassle.

hvasbath · 2017-06-24T08:26:51Z

Some of the tests exceed the time, and some dont give output for 10mins. I guess its related to what @ColCarroll mentioned above ...

junpenglao · 2017-06-24T08:31:07Z

Yeah ongoing issue... restarted them by hand for you.

hvasbath · 2017-06-27T06:21:05Z

What happened to the notebook? Ah I see got removed for the release ... I guess here only the tests need to be rerun after the theano caching trick they should run through...

fonnesbeck · 2017-06-27T07:07:08Z

#2354 puts the notebook back in. I will go ahead and merge it.

junpenglao · 2017-06-27T07:18:11Z

@hvasbath the conflict still need to resolved tho

junpenglao · 2017-06-27T10:27:29Z

Thanks @hvasbath!

Remove necessity to define likelihood variable inside model, is done …

8e7ac20

…now inside sampler init * added testing intermediate stage loading results * renamed ATMIP_sample to smc_sample * minor fix in clearing existing stages

junpenglao reviewed Jun 21, 2017

View reviewed changes

fonnesbeck reviewed Jun 21, 2017

View reviewed changes

hvasbath added 3 commits June 21, 2017 15:03

rename to sample_smc

60a7575

rename to sample_SMC, raise error of RV exists with llk_name, restruc…

2970d18

…ture test

fix test_step

19bf9bd

let MVNormal Proposal return 2d step matrix, SMC uses this, remove un…

8ea0fa5

…used import

twiecki reviewed Jun 23, 2017

View reviewed changes

sample name change again

e9cc38e

hvasbath mentioned this pull request Jun 24, 2017

SMC notebook broken #2351

Closed

hvasbath added 2 commits June 24, 2017 21:42

forgot the test_step, fixed ...

f97b8f4

restart travis

f5f3d24

Merge branch 'master' into smc_model_llk

cac698c

junpenglao merged commit 53fa46b into pymc-devs:master Jun 27, 2017

hvasbath deleted the smc_model_llk branch June 27, 2017 11:02

Uh oh!

Remove necessity to define likelihood variable inside model, is done … #2339

Remove necessity to define likelihood variable inside model, is done … #2339

Uh oh!

Conversation

hvasbath commented Jun 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvasbath Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twiecki Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvasbath commented Jun 21, 2017

Uh oh!

fonnesbeck commented Jun 21, 2017

Uh oh!

junpenglao commented Jun 21, 2017

Uh oh!

aseyboldt commented Jun 21, 2017

Uh oh!

hvasbath commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aseyboldt commented Jun 21, 2017

Uh oh!

hvasbath commented Jun 21, 2017

Uh oh!

ColCarroll commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColCarroll commented Jun 23, 2017 via email

Uh oh!

junpenglao commented Jun 23, 2017

Uh oh!

fonnesbeck commented Jun 23, 2017

Uh oh!

twiecki commented Jun 23, 2017

Uh oh!

fonnesbeck commented Jun 23, 2017

Uh oh!

hvasbath commented Jun 24, 2017

Uh oh!

junpenglao commented Jun 24, 2017

Uh oh!

hvasbath Jun 21, 2017 •

edited

Loading

twiecki Jun 21, 2017 •

edited

Loading

hvasbath commented Jun 21, 2017 •

edited

Loading

hvasbath commented Jun 27, 2017 •

edited

Loading