DimensionSet function doesn't work like expected.

[toeklk]

migrated from Bugzilla #463
status NEW severity enhancement in component core for ---
Reported in version trunk on platform All
Assigned to: BFL mailinglist

Original attachment names and IDs:

• discrete-DimensionSet.patch (ID 161)
• dimensionSet.patch (ID 179)

On 2007-12-06 22:27:02 +0100, François Cauwe wrote:

  Created attachment 161 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.

On 2007-12-07 09:43:59 +0100, Klaas Gadeyne wrote:

  (In reply to comment # 0) > Created an attachment (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 ColumnVectorampersand 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.

On 2007-12-07 09:50:57 +0100, Tinne De Laet wrote:

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

On 2007-12-07 10:14:17 +0100, Tinne De Laet wrote:

  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

On 2007-12-22 00:49:00 +0100, François Cauwe wrote:

  I propose to remove the function DimensionSet, like discussed in [1]. The only place where it is called outside the class itself is in: ./filterproposaldensity.cpp: _TmpPrior->DimensionSet(SysModel->StateSizeGet()); There it is meant for the initialisation of _TmpPrior. _TmpPrior is a gaussian, and no code really handles the dimensionSet function(only changes the variable). However this methode works because of: Gaussian::ExpectedValueSet (const ColumnVectorampersand mu) { _Mu = mu; if (this->DimensionGet() == 0) { this->DimensionSet(mu.rows()); } assert(this->DimensionGet() == mu.rows()); } If you first call DimensionSet(3) and then set the expectedValue with mu 1x3, it works, but the covariance might still be wrong. Also the line _TmpPrior->DimensionSet(SysModel->StateSizeGet()); is not needed for initialisation, because the first time you call ExpectedValueSet it will set the right dimension, as shown in bug # 465. [1] http://lists.mech.kuleuven.be/pipermail/bfl/2007-December/000822.html

On 2008-01-02 14:07:44 +0100, Tinne De Laet wrote:

  Created attachment 179 Changes the dimensionSet to a protected function In the proposed patch, I made the dimensionSet function protected. Like this _dimension stays private and the helper-function dimensionSet can also be called by the daughterclasses. Tinne

On 2008-01-02 17:13:26 +0100, François Cauwe wrote:

It seems fine for me.