#9670: Bring probability/random_variable.py to 100% coverage
-----------------------------+----------------------------------------------
Reporter: kcrisman | Owner: mvngu
Type: defect | Status: new
Priority: major | Milestone: sage-5.0
Component: documentation | Keywords:
Work_issues: | Upstream: N/A
Reviewer: | Author:
Merged: | Dependencies:
-----------------------------+----------------------------------------------
Changes (by was):
* type: enhancement => defect
Old description:
> Currently, this file is pretty woeful.
> {{{
> probability/random_variable.py: 3% (1 of 29)
> }}}
New description:
Currently, this file is pretty woeful.
{{{
probability/random_variable.py: 3% (1 of 29)
}}}
Also, there is at least one *enormous* bug in this code. Since this is
literally the first thing I tried, I'm guessing there may be many, many
others.
{{{
sage: n=6; P = dict([(i,1/n) for i in [1..n]])
sage: X = DiscreteProbabilitySpace(P.keys(), P)
sage: X.expectation() # WHAT?
0.166666666666667
}}}
--
Comment:
This file is a great example of "if it isn't tested, it's totally broken!"
I've never used any of this code before just now when I grad student
walked into my office and asked how to use it. He (fortunately) couldn't
even get started because typing {{{DiscreteProbabilitySpace?}}} in the
notebook provides absolutely no help at all (strangely, in the command
line, it does). So I looked at the code and came up with the first
trivial example:
{{{
sage: n=6; P = dict([(i,1/n) for i in [1..n]])
sage: X = DiscreteProbabilitySpace(P.keys(), P)
sage: X
Discrete probability space defined by {1: 1/6, 2: 1/6, 3: 1/6, 4: 1/6, 5:
1/6, 6: 1/6}
sage: X.expectation()
0.166666666666667
}}}
Literally, the first thing I tried came out completely wrong! That's sure
as hell not the expectation, which should be sum i*1/n = n*(n+1)/2/n =
(n+1)/2 = 7/2.
I then looked at the code for expectation. It's obviously got not test
(and don't worry kcrisman -- you didn't add any), and the code itself is
just wrong:
{{{
def expectation(self):
r"""
The expectation of the discrete random variable, namely
`\sum_{x \in S} p(x) X[x]`, where `X` = self and
`S` is the probability space of `X`.
"""
E = 0
Omega = self.probability_space()
for x in self._function.keys():
E += Omega(x) * self(x)
return E
}}}
I don't know why it does Omega=..., since self.probability_space() is self
is True.
I don't know why this doesn't use a single list comprehension line.
Here self._function is the dictionary: {{{{1: 1/6, 2: 1/6, 3: 1/6, 4: 1/6,
5: 1/6, 6: 1/6}}}}.
So the code should be:
{{{
def expectation(self):
return sum(x*self(x) for x in self._function.keys())
}}}
right?
Indeed,
{{{
sage: self=X; sum(x*self(x) for x in self._function.keys())
3.50000000000000
}}}
But that said, what's up with the floating point numbers? Shouldn't the
answer be exact?
{{{
sage: type(self(1))
<type 'sage.rings.real_mpfr.RealNumber'>
}}}
There is a way to deal with this:
{{{
sage: X = DiscreteProbabilitySpace(P.keys(), P, codomain=QQ)
sage: self=X; sum(x*self(x) for x in self._function.keys())
7/2
}}}
But shouldn't the default codomain depend on the dictionary P? It should
take the values and find a common parent for them (e.g., using Sequence),
then take the fraction field of that.
In defense of the original code, David K. wrote it before Sequence
existed, and we were just figuring out how to use Python at the time....
and didn't know if Sage would have more than 3 users.
Anyway, this code as is scary and needs massive, massive reworking.
And since I'm reporting a specific bug, I am changing this to a defect.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9670#comment:2>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
