[Bug 608] New: MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit
Summary: MCPdf forces constructor with unsigned int on T
Product: BFL
Version: trunk
Platform: All
OS/Version: All
Status: NEW
Severity: major
Priority: P3
Component: core
AssignedTo: bfl [..] ...
ReportedBy: meeussen [..] ...
CC: bfl [..] ...
Estimated Hours: 0.0

Hi,

When constructing an MCPdf, it is required that T has a constructor
T(unsigned int size). This makes only sense for types T that have a variable
size (MatrixWrapper::Columnvector, std::vector, ...), but not for types T that
have a fixed size (KDL::Twist, Bullet::Vector3, ...).

I think this constructor is now required to make filters with a variable size
type T realtime safe. But now it is not possible any more to use fixed size
type T any move, unless you wrap T, and add a bogus constructor with an
unsigned int. Especially for particle filters, you don't want to get any
performance hits from using either a variable size T, or a wrapper class.

I don't directly see any nice way to allow both realtime safe usage of variable
size T, and allow fixed size T. One possible half-solution would be to create
templete specializations for the BFL variable size T (ColumnVector, RowVector),
and not force the constructor with the unsigned int (this is what BFL used in
the past I think).

Wim

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

Wim Meeussen <meeussen [..] ...> changed:

What |Removed |Added
----------------------------------------------------------------------------
Resolution| |FIXED
Status|REOPENED |RESOLVED

--- Comment #20 from Wim Meeussen <meeussen [..] ...> 2009-03-11 19:17:52 ---
(In reply to comment #19)
> Patch of attachment #393 [details] applied in revision 30030
>
> Sending src/pdf/mcpdf.cpp
> Transmitting file data .
> Committed revision 30030.
>
> @Wim: Is the bug solved according to you?

I pulled in your changes of r 30030 and ran our tests successfully. This bug is
solved for me.

Thanks,

Wim

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

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

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

--- Comment #19 from Tinne De Laet <tinne [dot] delaet [..] ...> 2009-03-11 13:42:52 ---
Patch of attachment #393 applied in revision 30030

Sending src/pdf/mcpdf.cpp
Transmitting file data .
Committed revision 30030.

@Wim: Is the bug solved according to you?

Tinne

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #18 from Tinne De Laet <tinne [dot] delaet [..] ...> 2009-03-11 13:40:26 ---
Created an attachment (id=393)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=393)
Adds template specialization for ColumnVector for copy constructor of mcpdf to
avoil allocations@runtime

see description

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #17 from Wim Meeussen <meeussen [..] ...> 2009-03-06 23:06:09 ---
I moved our tree to the latest BFL revision, and all our tests run
successfully. Thanks for the patch!

Wim

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #16 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-03-06 16:23:53 ---
AFAIS the compile error is due to the fact that Tinne introduced a copy
constructor in the MCPdf class, which exhibits the same problem as the original
constructor. In r30024, I removed the malicious code (which makes the compile
error go away in my setup). However, this means that, for Tinnes new mixture
stuff, this will result in (unwanted) memory allocations. This can be fixed by
adding the necessary template specialisations for the copy constructor (as I
did for the constructor in the original patch).

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

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

What |Removed |Added
----------------------------------------------------------------------------
Resolution|FIXED |
Status|RESOLVED |REOPENED

--- Comment #15 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-03-05 18:29:40 ---
As I said. Untested :-(
Seems the changes that tinne has done in order to implement the Mixture Pdfs
break this patch.
Wim, you´ve waited to long to confirm it worked :-)

--
Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.
_______________________________________________
I hereby promise not to top-post on the
BFL mailing list
BFL [..] ...
http://lists.mech.kuleuven.be/mailman/listinfo/bfl

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

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

What |Removed |Added
----------------------------------------------------------------------------
Resolution| |FIXED
Status|ASSIGNED |RESOLVED

--- Comment #14 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-03-05 11:53:07 ---
Committed the patch in r30020 (since Tinne already modified some stuff in the
same location, I had to apply a fuzz factor). Hope this doesn't break anything
(can't test from here). Reopen if it would be the case.

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #13 from Wim Meeussen <meeussen [..] ...> 2009-03-04 22:27:14 ---
(In reply to comment #12)
> Created an attachment (id=374)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=374) [details]
> Better version

I already forgot we were testing your patch ;-) We've been running BFL with
your patch for quite a while now, and have not encountered any problems.

Wim

[Bug 608] New: MCPdf<T> forces constructor with unsigned int on

On 06 Jan 2009, at 19:08, Wim Meeussen wrote:

> For more infomation about this bug, visit <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=608
> >
> Summary: MCPdf<T> forces constructor with unsigned int on T
> Product: BFL
> Version: trunk
> Platform: All
> OS/Version: All
> Status: NEW
> Severity: major
> Priority: P3
> Component: core
> AssignedTo: bfl [..] ...
> ReportedBy: meeussen [..] ...
> CC: bfl [..] ...
> Estimated Hours: 0.0
>
> Hi,
>
> When constructing an MCPdf<T>, it is required that T has a constructor
> T(unsigned int size). This makes only sense for types T that have a
> variable
> size (MatrixWrapper::Columnvector, std::vector, ...), but not for
> types T that
> have a fixed size (KDL::Twist, Bullet::Vector3, ...).
>
> I think this constructor is now required to make filters with a
> variable size
> type T realtime safe. But now it is not possible any more to use
> fixed size
> type T any move, unless you wrap T, and add a bogus constructor with
> an
> unsigned int. Especially for particle filters, you don't want to get
> any
> performance hits from using either a variable size T, or a wrapper
> class.
>
> I don't directly see any nice way to allow both realtime safe usage
> of variable
> size T, and allow fixed size T. One possible half-solution would be
> to create
> templete specializations for the BFL variable size T (ColumnVector,
> RowVector),
> and not force the constructor with the unsigned int (this is what
> BFL used in
> the past I think).

I want to remark that previously we have been considering removing the
constructor Pdf<T>(unsigned int).
Since, for instance discretepdf.h misuses the constructor. There the
meaning of the unsigned int is different from the one given in the pdf-
base class. In discretepdf the unsigned int represents the number of
states while in the pdf it represent the dimension of the state space.
I recall somehow that a while ago I started the reimplementation of
all pdfs when there is no Pdf<T>(unsinged int) constructor but
somehow I failed to finish and commit it (don't remember why).

Please keep this problem in mind when proposing solutions for MCPdf<T>
constructors ....

Tinne

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 608] New: MCPdf<T> forces constructor with unsigned int on

On Mon, Jan 26, 2009 at 10:18 AM, Tinne De Laet
<tinne [dot] delaet [..] ...> wrote:
> On 06 Jan 2009, at 19:08, Wim Meeussen wrote:
>
>> For more infomation about this bug, visit
>> <https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=608>
>> Summary: MCPdf<T> forces constructor with unsigned int on T
>> Product: BFL
>> Version: trunk
>> Platform: All
>> OS/Version: All
>> Status: NEW
>> Severity: major
>> Priority: P3
>> Component: core
>> AssignedTo: bfl [..] ...
>> ReportedBy: meeussen [..] ...
>> CC: bfl [..] ...
>> Estimated Hours: 0.0
>>
>> Hi,
>>
>> When constructing an MCPdf<T>, it is required that T has a constructor
>> T(unsigned int size). This makes only sense for types T that have a
>> variable
>> size (MatrixWrapper::Columnvector, std::vector, ...), but not for types T
>> that
>> have a fixed size (KDL::Twist, Bullet::Vector3, ...).
>>
>> I think this constructor is now required to make filters with a variable
>> size
>> type T realtime safe. But now it is not possible any more to use fixed
>> size
>> type T any move, unless you wrap T, and add a bogus constructor with an
>> unsigned int. Especially for particle filters, you don't want to get any
>> performance hits from using either a variable size T, or a wrapper class.
>>
>> I don't directly see any nice way to allow both realtime safe usage of
>> variable
>> size T, and allow fixed size T. One possible half-solution would be to
>> create
>> templete specializations for the BFL variable size T (ColumnVector,
>> RowVector),
>> and not force the constructor with the unsigned int (this is what BFL used
>> in
>> the past I think).
>
> I want to remark that previously we have been considering removing the
> constructor Pdf<T>(unsigned int).
> Since, for instance discretepdf.h misuses the constructor. There the
> meaning of the unsigned int is different from the one given in the pdf-base
> class. In discretepdf the unsigned int represents the number of states while
> in the pdf it represent the dimension of the state space.
> I recall somehow that a while ago I started the reimplementation of all pdfs
> when there is no Pdf<T>(unsinged int) constructor but somehow I failed to
> finish and commit it (don't remember why).

You got pregnant in between? ;-)

> Please keep this problem in mind when proposing solutions for MCPdf<T>
> constructors ....

Agreed, but you can consider the (small) patch for this bug report as
an (innocent) bugfix that does not change the API or has implications
on the rest of the code, whereas the above "redesign suggestion" is
from a completely different order, since it affects the whole of BFL.

Klaas
_______________________________________________
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 608] MCPdf<T> forces constructor with unsigned int on T

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

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

What |Removed |Added
--------------------------------------------------------------------------
Attachment #374|Better version |Fix missing inline instead
description| |of altering the build system

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

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

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

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

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

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

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #12 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-23 20:10:32 ---
Created an attachment (id=374)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=374)
Better version

Hmm, seriously f*cked up the previous patch. Never trust "patch from
clipboard" :-)

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #11 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-23 20:07:30 ---
Created an attachment (id=373)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=373)
Fix missing inline without modifying the build system

AFAIR (but I don't remember it very well), your solution is not the good one,
since it will result in problems in other situations, see
<http://www.parashift.com/c++-faq-lite/templates.html#faq-35.12> and next
question. I do recall that templates and compilation is always tricky, and I
assume that the current build system was created after I spent some time.
However, I forgot to make the template specialisation "inline" in my patch,
which probably caused the multiply defined symbols
Does the attached patch help (fixed a missing "inline" statement)?

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

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

--- Comment #10 from Wim Meeussen <meeussen [..] ...> 2009-01-23 19:26:31 ---
Created an attachment (id=372)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=372)
same patch as previous one, but against latest svn

When mcpdf.cpp was included from mcpdf.hpp, I had linking problems because
multiple .o files contained the same implementation of the functions in
mcpdf.cpp. Therefore I removed the include in mcpdf.hpp, and added the
mcpdf.cpp file to the CMakeList.txt file.

Wim

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

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

What |Removed |Added
--------------------------------------------------------------------------
Status|NEW |ASSIGNED
Target Milestone|--- |0.6.2

--- Comment #9 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-14 20:02:36 ---
Can you explain why you changed this and rebase your patch against current SVN,
which makes it easier to review?

thx!

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #8 from Wim Meeussen <meeussen [..] ...> 2009-01-13 19:26:04 ---
Created an attachment (id=369)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=369)
corrected patch

I changed the patch, to compile the mcpdf.cpp, and not include it from the
header.

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #7 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-11 18:05:07 ---
Created an attachment (id=364)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=364)
mylyn/context/zip

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #6 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-11 18:05:05 ---
Created an attachment (id=363)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=363)
Try to fix it using partial template specialization

This was a quick hack. It removes the constructors that use the (unsigned int)
and adds an real-time safe implementation for ColumnVector.
This means that it's currently up to you to make sure that your implementation
won't malloc (e.g. by providing a template specialization yourself)
Certainly this could be implemented more cleanly (I leave this as an exercise)
Please test thoroughly. I doubt the current unit testing will be sufficient
:-(

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #4 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-11 12:03:51 ---
Created an attachment (id=361)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=361)
patch by Wim Meeussen that shows the problem

Attaching the patch here as an attachment, so I can integrate it easier in
eclipse (I hope)

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #5 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-11 12:03:53 ---
Created an attachment (id=362)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=362)
mylyn/context/zip

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #3 from Wim Meeussen <meeussen [..] ...> 2009-01-06 21:56:11 ---
The following patch to the pdf-test shows the problem:

[meeussen@amw ~/ros/ros-pkg/3rdparty/bfl/bfl-svn/tests]
$ svn diff
Index: pdf_test.cpp
===================================================================
--- pdf_test.cpp (revision 29808)
+++ pdf_test.cpp (working copy)
@@ -729,3 +729,24 @@
CPPUNIT_ASSERT_EQUAL(approxEqual(test_diff,
(Matrix)a_mcpdf_uint.CovarianceGet(),epsilon),true);
/* ProbabilityGet */
}
+
+
+
+class MyType
+{
+public:
+ // empty constructor
+ MyType() {};
+
+ // unsigned int constructor
+ //MyType(unsigned int dim) {};
+
+ ~MyType() {};
+};
+
+
+void
+PdfTest::testMcpdfType()
+{
+ MCPdf a_mcpdf(NUM_SAMPLES,DIMENSION);
+}
Index: pdf_test.hpp
===================================================================
--- pdf_test.hpp (revision 29808)
+++ pdf_test.hpp (working copy)
@@ -42,6 +42,7 @@
CPPUNIT_TEST( testLinearAnalyticConditionalGaussian );
CPPUNIT_TEST( testDiscreteConditionalPdf );
CPPUNIT_TEST( testMcpdf );
+ CPPUNIT_TEST( testMcpdfType );
CPPUNIT_TEST_SUITE_END();

ColumnVector _mu;
@@ -58,6 +59,7 @@
void testLinearAnalyticConditionalGaussian();
void testDiscreteConditionalPdf();
void testMcpdf();
+ void testMcpdfType();

private:
double epsilon;

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #2 from Wim Meeussen <meeussen [..] ...> 2009-01-06 21:45:39 ---
> > When constructing an MCPdf, it is required that T has a constructor
> > T(unsigned int size).

> Could you show some code snippets which illustrate the problem more precisely.
> E.g. in pdf_test.cpp I find
> tests/pdf_test.cpp: MCPdf
> and unsigned int has a fixed size?

For example, MCPdf, requires that the constructor "Twist(unsigned int)"
exists.

[Bug 608] MCPdf<T> forces constructor with unsigned int on T

For more infomation about this bug, visit

--- Comment #1 from Klaas Gadeyne <klaas [dot] gadeyne [..] ...> 2009-01-06 19:34:00 ---
(In reply to comment #0)
> Hi,
>
> When constructing an MCPdf, it is required that T has a constructor
> T(unsigned int size). This makes only sense for types T that have a variable
> size (MatrixWrapper::Columnvector, std::vector, ...), but not for types T that
> have a fixed size (KDL::Twist, Bullet::Vector3, ...).
>
> I think this constructor is now required to make filters with a variable size
> type T realtime safe. But now it is not possible any more to use fixed size
> type T any move, unless you wrap T, and add a bogus constructor with an
> unsigned int. Especially for particle filters, you don't want to get any
> performance hits from using either a variable size T, or a wrapper class.
>
> I don't directly see any nice way to allow both realtime safe usage of variable
> size T, and allow fixed size T. One possible half-solution would be to create
> templete specializations for the BFL variable size T (ColumnVector, RowVector),
> and not force the constructor with the unsigned int (this is what BFL used in
> the past I think).

Could you show some code snippets which illustrate the problem more precisely.
E.g. in pdf_test.cpp I find

tests/pdf_test.cpp: MCPdf

and unsigned int has a fixed size?