Skip to content
This repository has been archived by the owner on Mar 19, 2021. It is now read-only.

Stan's refactor #282

Merged
merged 23 commits into from Jan 14, 2017
Merged

Stan's refactor #282

merged 23 commits into from Jan 14, 2017

Conversation

syclik
Copy link
Member

@syclik syclik commented Dec 8, 2016

Summary:

This goes along with Stan's refactor. This updates PyStan so it's compatible with the changes to Stan.

Do not merge until stan-dev/stan#1751 is fixed.

Intended Effect:

Updates PyStan.

How to Verify:

rm pystan/*.so

python3 setup.py build_ext --inplace
python3
#exec(open("foo.py").read())


import pystan

schools_code = """
data {
    int<lower=0> J; // number of schools
    real y[J]; // estimated treatment effects
    real<lower=0> sigma[J]; // s.e. of effect estimates
}
parameters {
    real mu;
    real<lower=0> tau;
    real eta[J];
}
transformed parameters {
    real theta[J];
    for (j in 1:J)
    theta[j] = mu + tau * eta[j];
}
model {
    eta ~ normal(0, 1);
    y ~ normal(theta, sigma);
}
"""

schools_dat = {'J': 8,
               'y': [28,  8, -3,  7, -1,  1, 18, 12],
               'sigma': [15, 10, 16, 11,  9, 11, 10, 18]}



fit = pystan.stan(model_code=schools_code, data=schools_dat, verbose=True, seed=1, chains=1, algorithm = "NUTS", control = dict(adapt_engaged = True, metric = "dense_e"), sample_file = "abc.csv")

Side Effects:

None.

Documentation:

None.

Reviewer Suggestions:

@ariddell

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):

Columbia University

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@@ -139,7 +139,7 @@ def _format_number(num, n_signif_figures, max_width):
"""
if max_width < 6:
raise NotImplementedError("Guaranteed formatting in fewer than 6 characters not supported.")
if math.isnan(num):
if math.isnan(num) or math.isinf(num):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was failing when num was infinite. I don't know if this is the way to deal with this in python.

@@ -637,7 +637,12 @@ def _get_valid_stan_args(base_args=None):

args['init_radius'] = args.get('init_r', 2.0)
if (args['init_radius'] <= 0):
args['init'] = "0".encode('ascii')
args['init'] = b"0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit correctly initializes to 0. This was slightly different behavior in Stan for the same args, but doing this will make it the same from PyStan.

@@ -941,6 +946,7 @@ def _writable_sample_file(file, warn=True, wfun=None):
if wfun is None:
wfun = lambda x, y: '"{}" is not writable; use "{}" instead'.format(x, y)
dir = os.path.dirname(file)
dir = os.getcwd() if dir == '' else dir
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When file is just the name of a file, os.path.dirname(file) returns '', which was failing on the next call os.access(dir, os.W_OK). By checking if it's '' and returning the current working directory, this now allows the files to get written to the working directory. Is that ok?

@@ -21,8 +21,8 @@ def test_init_zero_exception_inf_grad(self):
"""
sm = StanModel(model_code=code)
assertRaisesRegex = self.assertRaisesRegexp if PY2 else self.assertRaisesRegex
with assertRaisesRegex(RuntimeError, 'Gradient evaluated at the initial value is not finite.'):
sm.sampling(init='0', iter=1)
with assertRaisesRegex(RuntimeError, ''):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, the error message from Stan is written as a message and not in the exception message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the message get to the user somehow? What's the new exception message? (If there's an exception message, that could be checked here.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure -- I think the error is handled slightly differently now. Wow... I don't even remember changing these tests. Do you want me to hunt down the difference?

@ariddell ariddell mentioned this pull request Dec 19, 2016
@syclik
Copy link
Member Author

syclik commented Dec 30, 2016

@ariddell, the code all runs, but I don't think I'm getting the output correctly propagated. Some of the output looks like all zeros.

But everything works. Can you review and let's talk about how we can get everything working?

@syclik
Copy link
Member Author

syclik commented Dec 30, 2016

by works, I mean that it runs and I get output back out to python.

@ariddell
Copy link
Contributor

ariddell commented Dec 31, 2016 via email

@syclik
Copy link
Member Author

syclik commented Dec 31, 2016 via email

@ariddell
Copy link
Contributor

ariddell commented Jan 9, 2017

Is this ready to review and merge?

@syclik
Copy link
Member Author

syclik commented Jan 9, 2017 via email

pystan.stan(model_code=model_code, iter=10, chains=1, seed=2,
data=data, init=[dict(mu=4)], warmup=0)
except RuntimeError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be redundant. a RuntimeError will yield a failing test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Please remove.

@ariddell
Copy link
Contributor

Great. I'll fix the tests and merge today.

@syclik
Copy link
Member Author

syclik commented Jan 10, 2017 via email

@ariddell
Copy link
Contributor

ariddell commented Jan 12, 2017

Hey @syclik, can we add a message to the throw domain_error here: https://github.com/stan-dev/stan/blob/1c2075bf8d0fe563b6907e40220aca016864e816/src/stan/services/util/initialize.hpp#L177

Python only knows that an exception has been encountered -- it doesn't know the exception type, so the "" isn't helpful. How about

throw std::domain_error("Initialization failed.");

edit: simplified things

@ariddell
Copy link
Contributor

ariddell commented Jan 12, 2017

I made a pull request, stan-dev/stan#2203

@syclik
Copy link
Member Author

syclik commented Jan 12, 2017 via email

@ariddell ariddell merged commit e9fc733 into develop Jan 14, 2017
@ariddell ariddell deleted the feature/refactor-services branch January 14, 2017 21:30
@ariddell
Copy link
Contributor

(Test failures were due to matplotlib not yet being available for Python 3.6)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants