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/(1Prob)
* 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

StatusASSIGNED RESOLVED
Resolution FIXED
Target Milestone 0.6.2
 Comment #7 from Tinne De Laet <tinne [dot] delaet [..] ...> 20080108 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 [..] ...> 20080108 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 is0 1
obsolete 
 Comment #5 from Tinne De Laet <tinne [dot] delaet [..] ...> 20080107 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 [..] ...> 20080107 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 unittests
> > >
> > >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 & v);.
> >
> > 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
> 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

StatusNEW ASSIGNED
AssignedTobfl [..] ... tinne [dot] delaet [..] ...en.b
 e
Attachment #178 is0 1
obsolete 
 Comment #3 from Tinne De Laet <tinne [dot] delaet [..] ...> 20080107 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 unittests
> >
> >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 & v);.
>
> 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
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 [..] ...> 20080104 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 unittests
>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 = (1a)/(1ProbabilityGet(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 [..] ...> 20080102 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 unittests
Any more comments, wishes?
Tinne