For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

--- Comment #1 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  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

For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

Klaas Gadeyne <klaas.gadeyne@fmtc.be> changed:

           What    |Removed                     |Added
 --------------------------------------------------------------------------
                 CC|                            |klaas.gadeyne@fmtc.be

--- Comment #2 from Klaas Gadeyne <klaas.gadeyne@fmtc.be>  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<Probability> & 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

For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

Tinne De Laet <tinne.delaet@mech.kuleuven.be> changed:

           What    |Removed                     |Added
 --------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|bfl@lists.mech.kuleuven.be  |tinne.delaet@mech.kuleuven.b
                   |                            |e
 Attachment #178 is|0                           |1
           obsolete|                            |

--- Comment #3 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  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<Probability> & 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<Probability>.

>
> 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).

For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

--- Comment #4 from Klaas Gadeyne <klaas.gadeyne@fmtc.be>  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<Probability> & 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<Probability>.

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

For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

Tinne De Laet <tinne.delaet@mech.kuleuven.be> changed:

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

--- Comment #5 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  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<Probability>.
>
> 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

For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

--- Comment #6 from Klaas Gadeyne <klaas.gadeyne@fmtc.be>  2008-01-08 08:58:56 ---

Patch seems fine for inclusion now.

For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=475>

Tinne De Laet <tinne.delaet@mech.kuleuven.be> changed:

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

--- Comment #7 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  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.