Skip to content

Conversation

fonnesbeck
Copy link
Member

This replaces #1430, which was merged by accident and reverted.

Though this does not directly solve #1428, it does make transformations more robust, and puts better default bounds (more conservative) on the scaling guess when none is provided.

mu = self.mu
return bound((-tau * (value - mu)**2 + tt.log(tau / np.pi / 2.)) / 2.,
tau > 0, sd > 0)
tau > 0)
Copy link
Member

Choose a reason for hiding this comment

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

I actually think this is required due to the sign disappearing when doing **-2

@springcoil
Copy link
Contributor

This gets my vote to merge. @twiecki what do you think?

@twiecki
Copy link
Member

twiecki commented Oct 21, 2016

I think this looks good. The new scaling min/max makes a bit weary though that we might be breaking models that have a huge scale on some values.

@fonnesbeck
Copy link
Member Author

That's why I included the scaling_bound argument -- so that it can be overridden. Certainly in the case of #1428 a zero was passed as tau, and 1e-10 was too small as a substitute. When it comes down to guessing the scaling, we should perhaps be happy with something that works, rather than an "ideal" value. Having a conservative scaling (i.e. larger for small values and smaller for large values) shouldnt break the sampler, should it?

@twiecki
Copy link
Member

twiecki commented Oct 22, 2016

Good point.

@twiecki twiecki merged commit 362c8ee into master Oct 22, 2016
@twiecki twiecki deleted the interval_fix branch October 22, 2016 14:55
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.

3 participants