Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge 3.1 branch #1658

Merged
merged 44 commits into from
Jan 18, 2017
Merged

Merge 3.1 branch #1658

merged 44 commits into from
Jan 18, 2017

Conversation

twiecki
Copy link
Member

@twiecki twiecki commented Jan 9, 2017

No description provided.

fonnesbeck and others added 30 commits January 9, 2017 16:51
…tting (e.g. nanguardmode) for Theano functions
* Started to write Base class for pymc3.models

* mode `add_var` to public api

* Added some docstrings

* Added some docstrings

* added getitem and fixed a typo

* added assertion check

* added resolve var method

* decided not to add resolve method

* Added linear component

* Docs fix

* patsy's intercept is inited properly now

* refactored code

* updated docs

* added possibility to init coefficients with random variables

* added glm

* refactored api, fixed formula init

* refactored linear model, extended acceptable types

* moved useful matrix and labels creation to utils file

* code style

* removed redundant evaluation of shape

* refactored resolver for constructing matrix and labels

* changed error message

* changed signature of init

* simplified utils any_to_tensor_and_labels code

* tests for `any_to_tensor_and_labels`

* added docstring for `any_to_tensor_and_labels` util

* forgot to document return type in `any_to_tensor_and_labels`

* refactored code for dict

* dict tests fix(do not check labels there)

* added access to random vars of model

* added a shortcut for all variables so there is a unified way to get them

* added default priors for linear model

* update docs for linear

* refactored UserModel api, made it more similar to pm.Model class

* Lots of refactoring, tests for base class, more plain api design

* deleted unused module variable

* fixed some typos in docstring

* Refactored pm.Model class, now it is ready for inheritance

* Added documentation for Model class

* Small typo in docstring

* nested contains for treedict (needed for add_random_variable)

* More accurate duplicate implementation of treedict/treelist

* refactored treedict/treelist

* changed `__imul__` of treelist

* added `root` property and `isroot` indicator for base model

* protect `parent` and `model` attributes from violation

* travis' python2 did not fail on bad syntax(maybe it's too new), fixed

* decided not to use functools wrapper

 Unfortunately functools wrapper fails
 when decorating built-in methods so I
 need to fix that improper behaviour.
 Some bad but needed tricks were implemented

* Added models package to setup script

* Refactor utils

* Fix some typos in pm.model
* Started to write Base class for pymc3.models

* mode `add_var` to public api

* Added some docstrings

* Added some docstrings

* added getitem and fixed a typo

* added assertion check

* added resolve var method

* decided not to add resolve method

* Added linear component

* Docs fix

* patsy's intercept is inited properly now

* refactored code

* updated docs

* added possibility to init coefficients with random variables

* added glm

* refactored api, fixed formula init

* refactored linear model, extended acceptable types

* moved useful matrix and labels creation to utils file

* code style

* removed redundant evaluation of shape

* refactored resolver for constructing matrix and labels

* changed error message

* changed signature of init

* simplified utils any_to_tensor_and_labels code

* tests for `any_to_tensor_and_labels`

* added docstring for `any_to_tensor_and_labels` util

* forgot to document return type in `any_to_tensor_and_labels`

* refactored code for dict

* dict tests fix(do not check labels there)

* added access to random vars of model

* added a shortcut for all variables so there is a unified way to get them

* added default priors for linear model

* update docs for linear

* refactored UserModel api, made it more similar to pm.Model class

* Lots of refactoring, tests for base class, more plain api design

* deleted unused module variable

* fixed some typos in docstring

* Refactored pm.Model class, now it is ready for inheritance

* Added documentation for Model class

* Small typo in docstring

* nested contains for treedict (needed for add_random_variable)

* More accurate duplicate implementation of treedict/treelist

* refactored treedict/treelist

* changed `__imul__` of treelist

* added `root` property and `isroot` indicator for base model

* protect `parent` and `model` attributes from violation

* travis' python2 did not fail on bad syntax(maybe it's too new), fixed

* decided not to use functools wrapper

 Unfortunately functools wrapper fails
 when decorating built-in methods so I
 need to fix that improper behaviour.
 Some bad but needed tricks were implemented

* Added models package to setup script

* Refactor utils

* Fix some typos in pm.model
* refactored GARCH and added Mv(Gaussian/StudentT)RandomWalk

* refactored garch logp

* added docs

* fix typo

* even more typos

* better description for tau
MAINT Replace discrete RV check.
…sses @ferrine's comments.

Remove static reference to theano.config.floatX. Use .astype(). Addresses @ferrine's comments.
@ferrine
Copy link
Member

ferrine commented Jan 14, 2017

in 7f95425 I fix cyclic import, you can do the same

@twiecki
Copy link
Member Author

twiecki commented Jan 14, 2017

Thanks @ferrine. Not sure why the exact sample test for HMC is failing here. @ColCarroll, ideas?

@ColCarroll
Copy link
Member

I feel like I've seen this exact failure before -- in the travis logs the traces look exactly the same (though I guess only the first 23 entries are actually the same). I can look more closely later, but probably need to print the full traces.

@twiecki
Copy link
Member Author

twiecki commented Jan 14, 2017

Most of them differ by E-20, but I saw one differing on order of .1.

@fonnesbeck
Copy link
Member

fonnesbeck commented Jan 17, 2017

If we are going to add a pymc3.models submodule, then we need to reconcile that with pymc3.model. We should not have both.

Also, we have things like timeseries, which is just another type of model, but it is not in models. This requires more discussion and consideration, but I don't think having a generic models submodule is the best way to go.

@ferrine
Copy link
Member

ferrine commented Jan 17, 2017

Can we try setuptools.find_packages() in setup script after fixing hmc? There were some issues with missing modules, they are easy to fix but setuptools suggest a better solution.

@ColCarroll
Copy link
Member

Ah, update on HMC -- I looked into it for a while, and took a larger sample from a working SHA and HEAD. The larger samples had the same behavior: they'd be equal for a while, then all of a sudden be different, the converge again, though there was no readily apparent pattern in how different they were. It is interesting because

  • even though HMC shares machinery with NUTS, only HMC is failing,
  • the momentum samples remain exactly the same
  • somehow the traces reconverge

The algorithm still seems fine -- my working theory is that the calls to the numpy random number generator are getting evaluated in a different order sometimes, or some other function is making a draw from the RNG. I'm not against replacing the test with the current trace, but I'll probably keep looking at this if only because I'm intrigued now.

@ColCarroll
Copy link
Member

In particular, I think the other call to an RNG is for the metropolis acceptance step, so it seems like that must be it...

@twiecki
Copy link
Member Author

twiecki commented Jan 17, 2017

@ColCarroll If that's the issue, does it suggest that the Metropolis accept does not use the correct seed? How do we fix it?

@ColCarroll
Copy link
Member

It'll take more testing to figure this out -- everything uses numpy.random, so the seed is set right. What I suspect happens is, roughly:

On master:

for step in steps:
    hmc_sample_momentum()
    hmc_metropolis_accept()
    # occasionally
    something_else_draw_sample()

In current branch:

On master:

for step in steps:
    hmc_sample_momentum()
    # occasionally
    something_else_draw_sample()
    hmc_metropolis_accept()

This would explain why it is usually in sync, but goes into and out of sync.

@twiecki
Copy link
Member Author

twiecki commented Jan 17, 2017

OK, so if we change the stored samples we should be good?

@ColCarroll
Copy link
Member

ColCarroll commented Jan 17, 2017 via email

@twiecki
Copy link
Member Author

twiecki commented Jan 17, 2017

@ferrine Can you push the update? Maybe we can decouple the easy resting from making travis happy. At least you looked deep enough that there aren't clear bugs.

travis is happy, the problem is still in hmc
@ferrine
Copy link
Member

ferrine commented Jan 17, 2017

I've merged my PR with find_packages into 3.1 but it did not appear here. Hmm, I did something wrong?
UPD: it appeared

@ColCarroll
Copy link
Member

wowza -- after some close checking, there is a transposed negative sign in the hmc sampler. Since hmc rarely rejects samples, the differences were very close. I'll add that change in.

Was computing negative energy for HMC.
The Metropolis tuning algorithm needed just a little nudge in the right direction.
@twiecki
Copy link
Member Author

twiecki commented Jan 18, 2017

@ColCarroll Wow, those tests really paid off here!

@twiecki
Copy link
Member Author

twiecki commented Jan 18, 2017

OK, I'm going to merge this so that we get out of this limbo.

@twiecki twiecki merged commit ce29d66 into master Jan 18, 2017
@fonnesbeck
Copy link
Member

Nice work guys!

@fonnesbeck fonnesbeck deleted the 3.1 branch June 25, 2017 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants