[Bug 475] New: ProbabilityGet/ Set of a discretePdf returns the weight of a state, not the normalize probability

For more infomation about this bug, visit
Summary: ProbabilityGet/Set of a discretePdf returns the weight
of a state, not the normalize probability
Product: BFL
Version: trunk
Platform: All
OS/Version: All
Status: NEW
Severity: normal
Priority: P2
Component: core
AssignedTo: bfl [..] ...
ReportedBy: francois [..] ...
CC: bfl [..] ...
Estimated Hours: 0.0

In the current implementation the of discrete pdf, the weight of a state is
returned instead of a probability. This can create some confusion and should be
explained in the documentation.

It could also been fixed, this implies:
* Add WeightSet(), which should set the weight of a state, not the probability
* Add WeightGet(), which should return the weight of a state.
* In ProbabilityGet, divide the weight by _SumWeights before returning the
value.
* In ProbabilitySet, set the new weight of the state on
Prob*_SumWeights/(1-Prob)
* Rename ProbabilitiesSet/Get to WeightsSet/Get

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

Tinne De Laet <tinne [dot] delaet [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution| |FIXED
Target Milestone|--- |0.6.2

--- Comment #7 from Tinne De Laet <tinne [dot] delaet [..] ...> 2008-01-08 09:05:07 ---
Patch applied in revision 28816.

Sending ChangeLog
Sending examples/discrete_filter/test_discrete_filter.cpp
Sending src/bfl_constants.h
Sending src/filter/histogramfilter.cpp
Sending src/pdf/discretepdf.cpp
Sending src/pdf/discretepdf.h
Sending tests/complete_filter_test.cpp
Sending tests/complete_filter_test.hpp
Sending tests/pdf_test.cpp
Transmitting file data .........
Committed revision 28816.

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

--- Comment #6 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-01-08 08:58:56 ---
Patch seems fine for inclusion now.

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

Tinne De Laet <tinne [dot] delaet [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
Attachment #180 is|0 |1
obsolete| |

--- Comment #5 from Tinne De Laet <tinne [dot] delaet [..] ...> 2008-01-07 17:01:50 ---
Created an attachment (id=181)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=181)
Patch to make sure that probability vector is normalized

[...]
> > Thanks for bringing this up.
> > To solve this issue a change the class variable _Values_p, containing the
> > probabilityvector from a ColumnVector to a vector.
>
> Seems ok to me (however, note my next bugreport :-). Is there any
> reason why it's still a pointer to a std::vector (which is rather
> uncommon), instead of a plain std::vector, which you can resize in the
> constructor?
I will try to change this when addressing your new bug :).

> Your current patch still contains the single line doxygen description
> "/// Only Relevant For Discrete Pdf's,", which might no be the best
> description of the probabilitySet method :-)
Oeps, I gave some more thorough description now :).

> So, due to the complexity, I'm not sure about the following:
> In the code for the historgram filter (lines 68 and 84), I have the
> impression that you still normalize the prob and new_prob variables.
> I would think those lines can be removed, since the normalization is
> done internally in discretepdf now?
You're right. I removed this normalization and simplified the code.

Tinne

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

--- Comment #4 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-01-07 16:24:01 ---
(In reply to comment #3)
> Created an attachment (id=180)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=180) [details]
> Patch to make sure that probability vector is normalized
>
> [...]
> > > I would like to propose a patch which, I hope, will solve the issue.
> > > I tried to make sure that the probability vector maintained in the
> > > discretepdf class is normalized all the time.
> > > Furthermore I tried to clarify some of the functions by adding some more
> > > descriptions and I added some unit-tests
> > >
> > >From reading the patch, I think the patch fails to address the following
> > > case:
> >
> > DiscretePdf p has 3 states with probabilities [1 0 0].
> > What will happen if I perform p.ProbabilitySet(2,0.5)?
> The function indeed failed for the call p.ProbabilitySet(1,0.5). The new patch
> solves this issue. Furthermore I added a test which explicitely tests the case
> where the probability changes was equal to 1.

OK. I was thinking about returning false in the above case, but your
solution seems a viable alternative. However, this should perhaps be
documented more explicitly in the doxygen documentation.

> > Furthermore, I think this class now somewhat "misuses" the Probability
> > class. The current implementation of this class (see bfl_constants.h)
> > performs some checks to ensure a Probability value is > 0, not inf. etc.
> > Actually the class is badly named, since it is not actually a Probability
> > value.
> Correct. I use this class to represent a number between 0 and 1. This was
> however the best solution I could think of. (adding some other "Probability"
> class will cause a lot of confusion IMHO)

Indeed.

[...]

> > A third issue is in the implementation of ProbabilitiesSet
> >
> > bool DiscretePdf::ProbabilitiesSet(ColumnVector & v)
> > {
> > assert(v.rows() == NumStatesGet());
> >
> > *_Values_p = v;
> > //normalize the probabilities and update the cumulative sum
> > return (NormalizeProbs() && CumPDFUpdate());
> > }
> >
> > If someone calls this function passing a ColumnVector with negative rows,
> > the function will return false, but the *_Values_p is still modified. This
> > means the DiscretePdf will be in an inconsistent state.
> > A possible solution would perhaps be to change the signature into
> >
> > bool DiscretePdf::ProbabilitiesSet(vector & v);
> Thanks for bringing this up.
> To solve this issue a change the class variable _Values_p, containing the
> probabilityvector from a ColumnVector to a vector.

Seems ok to me (however, note my next bugreport :-). Is there any
reason why it's still a pointer to a std::vector (which is rather
uncommon), instead of a plain std::vector, which you can resize in the
constructor?

> > E.g. for the probabilitySet function call
> >
> > /// Set the Probability of a discrete state
> > /** This function changes the probabilities such that AFTER
> > normalization the probability of a discrete state is equal
> > to the given probability.
> > @param state number of the discrete state of which the
> > probability will be set, (counting starts from 0, so state
> > should be >= 0 and < NumStatesGet())
> > @param a probability value to which the probability of
> > discrete state will be set (must be <= 1)
> > */
> > bool ProbabilitySet(unsigned int state, Probability a);

Your current patch still contains the single line doxygen description
"/// Only Relevant For Discrete Pdf's,", which might no be the best
description of the probabilitySet method :-)

> PS: I also changed some indentation, which can make the patch harder to read
> (sorry).

Indeed, the patch is very hard to read now, partly because I also
suggested some doxygen improvements and API changes which complicates
matters. All apologies :-(

So, due to the complexity, I'm not sure about the following:
In the code for the historgram filter (lines 68 and 84), I have the
impression that you still normalize the prob and new_prob variables.
I would think those lines can be removed, since the normalization is
done internally in discretepdf now?

regards,

Klaas

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

Tinne De Laet <tinne [dot] delaet [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
Status|NEW |ASSIGNED
AssignedTo|bfl [..] ... |tinne [dot] delaet [..] ...en.b
| |e
Attachment #178 is|0 |1
obsolete| |

--- Comment #3 from Tinne De Laet <tinne [dot] delaet [..] ...> 2008-01-07 10:29:54 ---
Created an attachment (id=180)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=180)
Patch to make sure that probability vector is normalized

[...]
> > I would like to propose a patch which, I hope, will solve the issue.
> > I tried to make sure that the probability vector maintained in the
> > discretepdf class is normalized all the time.
> > Furthermore I tried to clarify some of the functions by adding some more
> > descriptions and I added some unit-tests
> >
> >From reading the patch, I think the patch fails to address the following
> > case:
>
> DiscretePdf p has 3 states with probabilities [1 0 0].
> What will happen if I perform p.ProbabilitySet(2,0.5)?
The function indeed failed for the call p.ProbabilitySet(1,0.5). The new patch
solves this issue. Furthermore I added a test which explicitely tests the case
where the probability changes was equal to 1.

> Furthermore, I think this class now somewhat "misuses" the Probability
> class. The current implementation of this class (see bfl_constants.h)
> performs some checks to ensure a Probability value is > 0, not inf. etc.
> Actually the class is badly named, since it is not actually a Probability
> value.
Correct. I use this class to represent a number between 0 and 1. This was
however the best solution I could think of. (adding some other "Probability"
class will cause a lot of confusion IMHO)

> However, your patch contains some unnecessary checks
>
> @@ -78,7 +77,13 @@
> bool DiscretePdf::ProbabilitySet(unsigned int input, Probability a)
> {
> assert((int)input >= 0 && input < NumStatesGet());
> -
> + assert(a>=0 && a<=1);
> +
>
> This first assert is useless since already checked in the Probability
> class. I don't know what the optimal solution to this problem is but I
> think we should maybe document this (probably separate bugreport)
Removed

> A third issue is in the implementation of ProbabilitiesSet
>
> bool DiscretePdf::ProbabilitiesSet(ColumnVector & v)
> {
> assert(v.rows() == NumStatesGet());
>
> *_Values_p = v;
> //normalize the probabilities and update the cumulative sum
> return (NormalizeProbs() && CumPDFUpdate());
> }
>
> If someone calls this function passing a ColumnVector with negative rows,
> the function will return false, but the *_Values_p is still modified. This
> means the DiscretePdf will be in an inconsistent state.
> A possible solution would perhaps be to change the signature into
>
> bool DiscretePdf::ProbabilitiesSet(vector & v);
Thanks for bringing this up.
To solve this issue a change the class variable _Values_p, containing the
probabilityvector from a ColumnVector to a vector.

>
> Some further minor comments:
> - Doxygen documentation: Since the member function to call the number of
> discrete events that are possible is now called "NumStatesGet()", I
> suggest to rename the param named "input" with "state" (or "state_num") to
> increase clarity.
Done.
>
> E.g. for the probabilitySet function call
>
> /// Set the Probability of a discrete state
> /** This function changes the probabilities such that AFTER
> normalization the probability of a discrete state is equal
> to the given probability.
> @param state number of the discrete state of which the
> probability will be set, (counting starts from 0, so state
> should be >= 0 and < NumStatesGet())
> @param a probability value to which the probability of
> discrete state will be set (must be <= 1)
> */
> bool ProbabilitySet(unsigned int state, Probability a);
>
> - The statement "One of the probabilities is smaller than 0" seems to be a
> contradictio in terminis :-)
ok, removed.

> - You can use the @pre statement in the doxygen comment instead of
> // precondition: sum of probabilities should be 1
Obsolete.

Tinne

PS: I also changed some indentation, which can make the patch harder to read
(sorry).

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

Klaas Gadeyne <klaas [dot] gadeyne [..] ...> changed:

What |Removed |Added
--------------------------------------------------------------------------
CC| |klaas [dot] gadeyne [..] ...

--- Comment #2 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2008-01-04 15:51:54 ---
(In reply to comment #1)
> Created an attachment (id=178)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=178) [details]
> Patch to make sure that probability vector is normalized
>
> First of all thanks for bringing up the issue.
>
> I would like to propose a patch which, I hope, will solve the issue.
> I tried to make sure that the probability vector maintained in the discretepdf
> class is normalized all the time.
> Furthermore I tried to clarify some of the functions by adding some more
> descriptions and I added some unit-tests

>From reading the patch, I think the patch fails to address the following case:

DiscretePdf p has 3 states with probabilities [1 0 0].
What will happen if I perform p.ProbabilitySet(2,0.5)?

Tip: Look at the line below in the patch :-)

+ double normalization_factor = (1-a)/(1-ProbabilityGet(input));

Furthermore, I think this class now somewhat "misuses" the Probability class.
The current implementation of this class (see bfl_constants.h) performs some
checks to ensure a Probability value is > 0, not inf. etc.
Actually the class is badly named, since it is not actually a Probability
value.
However, your patch contains some unnecessary checks

@@ -78,7 +77,13 @@
bool DiscretePdf::ProbabilitySet(unsigned int input, Probability a)
{
assert((int)input >= 0 && input < NumStatesGet());
-
+ assert(a>=0 && a<=1);
+

This first assert is useless since already checked in the Probability class.
I don't know what the optimal solution to this problem is but I think we should
maybe document this (probably separate bugreport)

A third issue is in the implementation of ProbabilitiesSet

bool DiscretePdf::ProbabilitiesSet(ColumnVector & v)
{
assert(v.rows() == NumStatesGet());

*_Values_p = v;
//normalize the probabilities and update the cumulative sum
return (NormalizeProbs() && CumPDFUpdate());
}

If someone calls this function passing a ColumnVector with negative rows, the
function will return false, but the *_Values_p is still modified. This means
the DiscretePdf will be in an inconsistent state.
A possible solution would perhaps be to change the signature into

bool DiscretePdf::ProbabilitiesSet(vector & v);

Some further minor comments:
- Doxygen documentation: Since the member function to call the number of
discrete events that are possible is now called "NumStatesGet()", I suggest
to rename the param named "input" with "state" (or "state_num") to increase
clarity.

E.g. for the probabilitySet function call

/// Set the Probability of a discrete state
/** This function changes the probabilities such that AFTER
normalization the probability of a discrete state is equal
to the given probability.
@param state number of the discrete state of which the
probability will be set, (counting starts from 0, so state
should be >= 0 and < NumStatesGet())
@param a probability value to which the probability of
discrete state will be set (must be <= 1)
*/
bool ProbabilitySet(unsigned int state, Probability a);

- The statement "One of the probabilities is smaller than 0" seems to be a
contradictio in terminis :-)

- You can use the @pre statement in the doxygen comment instead of
// precondition: sum of probabilities should be 1

[Bug 475] ProbabilityGet/Set of a discretePdf returns the weight

For more infomation about this bug, visit

--- Comment #1 from Tinne De Laet <tinne [dot] delaet [..] ...> 2008-01-02 13:16:45 ---
Created an attachment (id=178)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=178)
Patch to make sure that probability vector is normalized

First of all thanks for bringing up the issue.

I would like to propose a patch which, I hope, will solve the issue.
I tried to make sure that the probability vector maintained in the discretepdf
class is normalized all the time.
Furthermore I tried to clarify some of the functions by adding some more
descriptions and I added some unit-tests

Any more comments, wishes?

Tinne