[Bug 473] New: Add & Remove state for DiscretePdf

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

           Summary: Add & Remove state for DiscretePdf
           Product: BFL
           Version: trunk
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: core
        AssignedTo: bfl [..] 
        ReportedBy: francois [..] 
                CC: bfl [..] 
   Estimated Hours: 0.0

Add the possibility to add and remove a component in discretePdf.

This is needed for the Mixture of Gaussian as discussed in:
http://lists.mech.kuleuven.be/pipermail/bfl/2007-December/000822.html

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.

[Bug 473] Add & Remove state for DiscretePdf

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

François Cauwe <francois [..] > changed:

           What    |Removed                     |Added
 --------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|bfl [..]   |francois [..] 
--- Comment #1 from François Cauwe <francois [..] >  2007-12-21 00:23:43 ---

Created an attachment (id=174)

 --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=174)

Patch

This patches adds this functionality, and also updates the unit tests.

[Bug 473] Add & Remove state for DiscretePdf

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

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

           What    |Removed                     |Added
 --------------------------------------------------------------------------
                 CC|                            |klaas gadeyne [..] 
--- Comment #2 from Klaas Gadeyne <klaas gadeyne [..] >  2007-12-21 08:49:10 ---

(In reply to comment #1)
> Created an attachment (id=174)

 --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=174) details

> Patch
> This patches adds this functionality, and also updates the unit tests.

Patch seems ok. 2 small remarks:

- Try generating your patches using svn diff -x -u, so they are somewhat more

readable.

- I think you should make the documentation about stateAdd somewhat more

explicit, in casu about the meaning you attach to the parameter "probability
p". As I understand from your patch, this is the probability of the new
discrete state _after_ renormalizing? Anyway, from your doxygen comment, this
is not clear to me.
As an example, suppose I have a discretePdf with
state 0: prob 0.5
state 1: prob 0.5

When I now perform a pdf.stateAdd(0.5), this will result in

state 0: prob 0.25
state 1: prob 0.25
state 2: prob 0.5

However, I could also expect it to be
state 0: prob 0.33
state 1: prob 0.33
state 2: prob 0.33

You should be more explicit in the docs about this behaviour I guess.
Thx for your contribution!

[Bug 473] Add & Remove state for DiscretePdf

...
> Patch seems ok. 2 small remarks:
> - Try generating your patches using svn diff -x -u, so they are somewhat
> more readable.
> - I think you should make the documentation about stateAdd somewhat more
> explicit, in casu about the meaning you attach to the parameter
> "probability p". As I understand from your patch, this is the probability
> of the new discrete state _after_ renormalizing? Anyway, from your doxygen
> comment, this is not clear to me.
>
> You should be more explicit in the docs about this behaviour I guess.
I agree with Klaas. It is very important, certainly in this case, to describe
what your functions are doing. For adding and removing a state there are many
ways in which the probabilities could change, so please describe carefully!
Furthermore, I think you should add some extra tests to the unit tests, to
check if the probabilities of all states (and not only the one added) changed
as expected (they should still be normalized..).
(so add unit tests to test all probabilities after you added or removed a
state.).

Keep on the good work!

Tinne

PS: Could you also add some lines in the Changelog describing your
enhancement?

Disclaimer: http://www.kuleuven.be/cwis/email_disclaimer.htm

___
I hereby promise not to top-post on the
BFL mailing list
BFL [..]
http://lists.mech.kuleuven.be/mailman/listinfo/bfl

Disclaimer: http://www.kuleuven.be/cwis/email_disclaimer.htm

[Bug 473] Add & Remove state for DiscretePdf

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

François Cauwe <francois [..] > changed:

           What    |Removed                     |Added
 --------------------------------------------------------------------------
 Attachment #174 is|0                           |1
           obsolete|                            |
--- Comment #3 from François Cauwe <francois [..] >  2007-12-22 00:16:05 ---

Created an attachment (id=176)

 --> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=176)

Updated patch

Thanks for the feedback, I made a new patch and:

 * Added a Changelog message [1]
 * Make a better unit test, checked all states.
 * Change the variable names, so it won't create any confusion with

probability. See also bug #475

[1] http://lists.mech.kuleuven.be/pipermail/bfl/2007-December/000868.html

[Bug 473] Add & Remove state for DiscretePdf

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

--- Comment #4 from François Cauwe <francois [..] >  2008-01-13 18:22:19 ---

Are there still some comments on this patch?

[Bug 473] Add & Remove state for DiscretePdf

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

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

           What    |Removed                     |Added
 --------------------------------------------------------------------------
                 CC|                            |tinne delaet [..] en.b
                   |                            |e
--- Comment #5 from Tinne De Laet <tinne delaet [..] >  2008-01-16 10:30:52 ---

Francois,

You should update your patch according to the latest changes to the trunk (see
bug #475.)

Tinne