BUG: solved issue with ppf #216

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@ndvanforeest
Contributor

ndvanforeest commented May 17, 2012

I issued this first as a branch called ticket1493. This got messed up with the branch repair-typo. Therefore I removed ticket1493, and pushed this new branch. There are also some comments related to the old, but now removed, branch. I include this as one comment here.

@ndvanforeest

This comment has been minimized.

Show comment
Hide comment
@ndvanforeest

ndvanforeest May 17, 2012

Contributor

I replaced the ppf function. Once this works, I'll remove xa and xb.

josef-pkt commented 8 hours ago
the new ppf function looks good, as discussed on the mailing list, the existing test should cover this case.

possible extra optimization
_ppf_to_solve uses cdf which needs to go through the generic wrapping code each time.
we could use _cdf instead, but that would require additional checking/tests to see whether check_args are properly set (I think they are)

nokfi commented 2 hours ago
As a test I naively replaced self.cdf by self._pdf in _ppf_to_solve, and ran test_continuous_basic.py This fails, so a simple replacement will not suffice.

josef-pkt commented 2 hours ago
"replaced self.cdf by self._pdf" did you actually use _pdf or _cdf ?

my guess was, that, since you limit the range to the interval [.a, .b], _cdf might work, but maybe it would require open intervall (.a, .b).
I tried some cases like this in the past, and sometimes it worked often it broke on a few distributions.

nokfi commented 2 hours ago
On 17 May 2012 21:30, Josef Perktold
reply@reply.github.com
wrote:
"replaced self.cdf by self._pdf" did you actually use _pdf or _cdf ?
To be explicit, I did this:

def _ppf_to_solve(self, x, q,*args):
    return apply(self._cdf, (x, )+args)-q

this self._cdf rather than self.cdf

my guess was, that, since you limit the range to the interval [.a, .b], _cdf might work, but maybe it would require open intervall (.a, .b).
I tried some cases like this in the past, and sometimes it worked often it broke on a few distributions.
I like to include .a and .b in the search interval to cover the cases
q = 0 and q =1. Sure, mathematically speaking the return value of
cdf(x) = 1 can be set to np.inf. However, this is, at least for me,
less informative than self.b. (and likewise for self.a).

So we stick to cdf, rather than _cdf, at least for the moment?
�$B!D

josef-pkt commented an hour ago
Yes, stick with .cdf. Worth a try, but if it doesn't work it's unrelated to current pull request.
Your limiting to [a,b] is the right thing to do

(aside: q=0, q=1 is supposed to be handled by generic part, but using left=.a and right=.b if finite, let's brentq search arbitrarily close to the boundary, which is singular in some cases (pdf goes to inf). Although, it might still break at a singularity, or if cdf uses intquad and we want ppf(1e-10))

Contributor

ndvanforeest commented May 17, 2012

I replaced the ppf function. Once this works, I'll remove xa and xb.

josef-pkt commented 8 hours ago
the new ppf function looks good, as discussed on the mailing list, the existing test should cover this case.

possible extra optimization
_ppf_to_solve uses cdf which needs to go through the generic wrapping code each time.
we could use _cdf instead, but that would require additional checking/tests to see whether check_args are properly set (I think they are)

nokfi commented 2 hours ago
As a test I naively replaced self.cdf by self._pdf in _ppf_to_solve, and ran test_continuous_basic.py This fails, so a simple replacement will not suffice.

josef-pkt commented 2 hours ago
"replaced self.cdf by self._pdf" did you actually use _pdf or _cdf ?

my guess was, that, since you limit the range to the interval [.a, .b], _cdf might work, but maybe it would require open intervall (.a, .b).
I tried some cases like this in the past, and sometimes it worked often it broke on a few distributions.

nokfi commented 2 hours ago
On 17 May 2012 21:30, Josef Perktold
reply@reply.github.com
wrote:
"replaced self.cdf by self._pdf" did you actually use _pdf or _cdf ?
To be explicit, I did this:

def _ppf_to_solve(self, x, q,*args):
    return apply(self._cdf, (x, )+args)-q

this self._cdf rather than self.cdf

my guess was, that, since you limit the range to the interval [.a, .b], _cdf might work, but maybe it would require open intervall (.a, .b).
I tried some cases like this in the past, and sometimes it worked often it broke on a few distributions.
I like to include .a and .b in the search interval to cover the cases
q = 0 and q =1. Sure, mathematically speaking the return value of
cdf(x) = 1 can be set to np.inf. However, this is, at least for me,
less informative than self.b. (and likewise for self.a).

So we stick to cdf, rather than _cdf, at least for the moment?
�$B!D

josef-pkt commented an hour ago
Yes, stick with .cdf. Worth a try, but if it doesn't work it's unrelated to current pull request.
Your limiting to [a,b] is the right thing to do

(aside: q=0, q=1 is supposed to be handled by generic part, but using left=.a and right=.b if finite, let's brentq search arbitrarily close to the boundary, which is singular in some cases (pdf goes to inf). Although, it might still break at a singularity, or if cdf uses intquad and we want ppf(1e-10))

@dlax

This comment has been minimized.

Show comment
Hide comment
@dlax

dlax May 18, 2012

Member

Does this replace PR #214? If so, you should close the latter @nokfi.

Member

dlax commented May 18, 2012

Does this replace PR #214? If so, you should close the latter @nokfi.

@ndvanforeest

This comment has been minimized.

Show comment
Hide comment
@ndvanforeest

ndvanforeest May 18, 2012

Contributor

Yes it does. I'll try to close #214.

On 18 May 2012 16:52, Denis Laxalde
reply@reply.github.com
wrote:

Does this replace PR #214? If so, you should close the latter @nokfi.


Reply to this email directly or view it on GitHub:
#216 (comment)

Contributor

ndvanforeest commented May 18, 2012

Yes it does. I'll try to close #214.

On 18 May 2012 16:52, Denis Laxalde
reply@reply.github.com
wrote:

Does this replace PR #214? If so, you should close the latter @nokfi.


Reply to this email directly or view it on GitHub:
#216 (comment)

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers May 20, 2012

Member

general_cont_ppf can indeed be removed.

Member

rgommers commented May 20, 2012

general_cont_ppf can indeed be removed.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers May 20, 2012

Member

I didn't see the git question in PR-214 before, but I'm working on disentangling some of these PRs now.

Nicky, I think you are now used to creating different branches for new features, but keep in mind that you should create each branch separately based on master, and not on each other (you did that correctly for this PR now). Otherwise the same commits show up in several PRs.

Member

rgommers commented May 20, 2012

I didn't see the git question in PR-214 before, but I'm working on disentangling some of these PRs now.

Nicky, I think you are now used to creating different branches for new features, but keep in mind that you should create each branch separately based on master, and not on each other (you did that correctly for this PR now). Otherwise the same commits show up in several PRs.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers May 20, 2012

Member

The change to test_continuous_basic.py is already in master, that's why this can't be merged automatically. Once this is done, I can rebase and merge it.

Member

rgommers commented May 20, 2012

The change to test_continuous_basic.py is already in master, that's why this can't be merged automatically. Once this is done, I can rebase and merge it.

@ndvanforeest

This comment has been minimized.

Show comment
Hide comment
@ndvanforeest

ndvanforeest May 20, 2012

Contributor

Sorry for the mess. Is there something that I should do about this, or is it easy to fix?

Contributor

ndvanforeest commented May 20, 2012

Sorry for the mess. Is there something that I should do about this, or is it easy to fix?

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers May 20, 2012

Member

No, it's no problem. This one's not too bad, PR-205 was the one requiring some surgery and that's merged now.

Member

rgommers commented May 20, 2012

No, it's no problem. This one's not too bad, PR-205 was the one requiring some surgery and that's merged now.

@ndvanforeest

This comment has been minimized.

Show comment
Hide comment
@ndvanforeest

ndvanforeest May 20, 2012

Contributor

Thanks for solving it.

On 20 May 2012 22:11, Ralf Gommers
reply@reply.github.com
wrote:

No, it's no problem. This one's not too bad, PR-205 was the one requiring some surgery and that's merged now.


Reply to this email directly or view it on GitHub:
#216 (comment)

Contributor

ndvanforeest commented May 20, 2012

Thanks for solving it.

On 20 May 2012 22:11, Ralf Gommers
reply@reply.github.com
wrote:

No, it's no problem. This one's not too bad, PR-205 was the one requiring some surgery and that's merged now.


Reply to this email directly or view it on GitHub:
#216 (comment)

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Jun 3, 2012

Member

docstring line 879 and following still contain explanation for xa xb AFAICS

The only problem left here is the removal of xa and xb from the init. This can possibly break existing code even if it's not necessary anymore.

I'm a bit puzzled that I don't find any xa, xb defined in any of the distributions when the instances are created. A quick look at my own code, also doesn't use it anymore. I was convinced that I changed xa and xb for some distributions.

If we want to be conservative with respect to backwards compatibility, we could add for example a **kwds to the init method, and warn,

if 'xa' in kwds or 'xb' in kwds: 
    import warnings
    <deprecation warning....>

Since I recently advertised xa, xb on stackoverflow, http://stackoverflow.com/questions/10678546/creating-new-distributions-in-scipy it might be better to raise a deprecation warning instead of raising an exception if someone defines xa, xb.

Other than the xa,xb in the init it's a good change. If the test suite passes (including slow tests), then there are no hidden problems with any distribution.

Member

josef-pkt commented Jun 3, 2012

docstring line 879 and following still contain explanation for xa xb AFAICS

The only problem left here is the removal of xa and xb from the init. This can possibly break existing code even if it's not necessary anymore.

I'm a bit puzzled that I don't find any xa, xb defined in any of the distributions when the instances are created. A quick look at my own code, also doesn't use it anymore. I was convinced that I changed xa and xb for some distributions.

If we want to be conservative with respect to backwards compatibility, we could add for example a **kwds to the init method, and warn,

if 'xa' in kwds or 'xb' in kwds: 
    import warnings
    <deprecation warning....>

Since I recently advertised xa, xb on stackoverflow, http://stackoverflow.com/questions/10678546/creating-new-distributions-in-scipy it might be better to raise a deprecation warning instead of raising an exception if someone defines xa, xb.

Other than the xa,xb in the init it's a good change. If the test suite passes (including slow tests), then there are no hidden problems with any distribution.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Jun 3, 2012

Member

Better to just not remove xa and xb keywords from the signature in that case, add the deprecation warning for it and open a new ticket to remove them for 0.12. Adding a new **kwds parameter is not so nice.

Member

rgommers commented Jun 3, 2012

Better to just not remove xa and xb keywords from the signature in that case, add the deprecation warning for it and open a new ticket to remove them for 0.12. Adding a new **kwds parameter is not so nice.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Jun 3, 2012

Member

I can do that, remove the left over doc and also remove general_cont_ppf and merge this.

Member

rgommers commented Jun 3, 2012

I can do that, remove the left over doc and also remove general_cont_ppf and merge this.

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Jun 3, 2012

Member

sounds good,
(I didn't like **kdws much either, but it would have removed xa, xb from the visible signature, although almost no one will look at the init signature)

Member

josef-pkt commented Jun 3, 2012

sounds good,
(I didn't like **kdws much either, but it would have removed xa, xb from the visible signature, although almost no one will look at the init signature)

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Jun 3, 2012

Member

in case it's not obvious: if xa, xb stay as kwd arguments, the defaults should be set to None, so it's easy to check whether someone used a value as argument.

Member

josef-pkt commented Jun 3, 2012

in case it's not obvious: if xa, xb stay as kwd arguments, the defaults should be set to None, so it's easy to check whether someone used a value as argument.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Jun 3, 2012

Member

Opened http://projects.scipy.org/scipy/ticket/1667 to track removal of these params.

Member

rgommers commented Jun 3, 2012

Opened http://projects.scipy.org/scipy/ticket/1667 to track removal of these params.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Jun 3, 2012

Member

recipinvgauss and foldcauchy were still initialized with an xb parameter by the way.

Member

rgommers commented Jun 3, 2012

recipinvgauss and foldcauchy were still initialized with an xb parameter by the way.

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Jun 3, 2012

Member

I'm glad you found them, I searched everywhere but only for xa, not for xb. (and I can still trust my memory)

Member

josef-pkt commented Jun 3, 2012

I'm glad you found them, I searched everywhere but only for xa, not for xb. (and I can still trust my memory)

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Jun 3, 2012

Member

foldcauchy doesn't seem to have a test for this specific issue though. recipinvgauss apparently wasn't working anyway, according to the FIXME note above it.

Member

rgommers commented Jun 3, 2012

foldcauchy doesn't seem to have a test for this specific issue though. recipinvgauss apparently wasn't working anyway, according to the FIXME note above it.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Jun 3, 2012

Member

Pushed as ed7f037 and 49ed1d2. Thanks Nicky and Josef.

Member

rgommers commented Jun 3, 2012

Pushed as ed7f037 and 49ed1d2. Thanks Nicky and Josef.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment