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

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

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

--- Comment #1 from Klaas Gadeyne <klaas gadeyne [..] >  2007-12-07 09:43:59 ---

(In reply to comment #0)
> Created an attachment (id=161)

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

> Patch that fixes this for discretepdf
>
> The function DimensionSet only changes the variable _dimension of a pdf, but
> never changes the dimension of a pdf.
> The included patch changes the dimension of a discretepdf, and not only the
> variable _dimension.

patch seems ok. However, this patch raises some additional questions to me.

1/ This resize() is a non-realtime operation. If the maintainer decides to
allow it (see 3/), you should at least add a doxygen comment to warn users that
this is a non-realtime operation.

2/ If your patch is valid, please add a unit test to bfl that only succeeds
using your patch and fails without.

2/ I wonder whether the DimensionSet() call should really be in the public API
of pdf? Can you describe the use case in which you would use the
DimensionSet() call.

Furthermore, while investigating this patch, I noticed the following code in
gaussian.cpp

 11978   kgadeyne   void
 11978   kgadeyne   Gaussian::ExpectedValueSet (const ColumnVector& mu)
 11978   kgadeyne   {
 12218   kgadeyne     _Mu = mu;
 27989   wmeeusse     assert(this->DimensionGet() == mu.rows());
 11992   kgadeyne     if (this->DimensionGet() == 0)
 11992   kgadeyne       {
 11992   kgadeyne       this->DimensionSet(mu.rows());
 11992   kgadeyne       }
 11978   kgadeyne   }

It seems to me the assert() renders the if statement invalid. Moreover, if it
should be there, I guess it's better places _before_ the assignment.
IIRC, the if statement is in place to be able to write something like

Gaussian g; // without args, dimension = 0
ColumnVector v(5) = 0.0;
g.ExpectedValueSet(v); // Non-realtime operation, change dimension to 5.

With the assert in place, this won't work.

However, _without_ the assert, the following won't raise an error

Gaussian g;
ColumnVector v(2)
SymmetricMatrix s(3);
g.ExpectedValueSet(v);
g.CovarianceSet(s);

which is not good either.

At first sight, this is solved by reordering the statements.

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

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

           What    |Removed                     |Added
 --------------------------------------------------------------------------
                 CC|                            |tinne delaet [..] en.b
                   |                            |e

--- Comment #2 from Tinne De Laet <tinne delaet [..] >  2007-12-07 09:50:57 ---

>> This was written simultaneously with Klaas' comment.

First I want to open a discussion on the dimensionSet function itself.
As implemented now, the dimensionSet function is a public function (with
template code in pdf.h).
The only time so far that dimensionSet is used as a public function is in
filterproposaldensity.cpp:
_TmpPrior->DimensionSet(SysModel->StateSizeGet());

I believe it is safer to make dimensionSet protected, but maybe we could
discuss the impact of this proposal. Sure, the above function call in
filterproposaldensity.cpp should be changed.

I want to make the dimensionSet protected since in my viewpoint it is a
function which the user should not be able to call. Imagine that while he's
doing some serieous estimation he suddenly calls dimensionset. The underlying
pdf-representation (Gaussian,discretepdf) is not adapted while doing this and
furthermore, it is very unclear HOW it should be adapted.

Any comments, suggestions?

Tinne

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

--- Comment #3 from Tinne De Laet <tinne delaet [..] >  2007-12-07 10:14:17 ---

Some additional thoughts,

If the dimensionSet is used as a public function, it seems to me it's only
usefull in the case the pdf under consideration is not initialized yet.
Therefor, it could also be usefull to add a private boolean _initialized,
representing if the pdf is initialized or not.
The dimensionSet could then be changed such that it is only possible to call
this function provided _initialized == false.

Remarks?

Tinne