[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit

wim [dot] meeussen [..] ... changed:

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

------- Comment #1 from wim [dot] meeussen [..] ... 2007-05-03 15:26

replaced checks and cerr by assert, so the checks can be disabled. See
Revision: 27980

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #9 from klaas [dot] gadeyne [..] ... 2007-05-25 10:51

(In reply to comment #8)
> (In reply to comment #5)
> > > What are problems we want to recover from?

[...]
> I think it's not necessary to recover from a negative weight.

You are right. The example Wim just provided is a much better one.

regards,
Klaas

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit

klaas [dot] gadeyne [..] ... changed:

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

------- Comment #2 from klaas [dot] gadeyne [..] ... 2007-05-03 15:38

(In reply to comment #1)
> replaced checks and cerr by assert, so the checks can be disabled. See
> Revision: 27980

I think you are refering to revision 27989?

I don't think this is a better solution. Please reread the bug report title:
Use exceptions or return codes. We should allow the layers above trying to
recover from this type of errors. E.g. people using a filter in their embedded
controller don't want their controller software to abandon all efforts because
their filter doesn't work.

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #14 from klaas [dot] gadeyne [..] ... 2007-05-25 13:47

(In reply to comment #6)
> Created an attachment (id=130)
--> (http://www.fmtc.be/orocos-bugzilla/attachment.cgi?id=130&action=view) [details]
> patch
>
> Patch to recover from case where all weights become zero. This can happen when
> the sensor measurements are totally incompatible with the prior of the filter.
> In case this happens, the update() function will return false.

Note: this patch does _not_ update API documentation...

/// Set the list of weighted samples
/** @param list_of_samples an STL-list containing the list of all
weighted samples
*/
bool ListOfSamplesSet(const vector > &
list_of_samples);

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #13 from klaas [dot] gadeyne [..] ... 2007-05-25 13:46

(In reply to comment #12)
> > > I'll close this bug now. If you find any new cases from which BFL should
> > > recover, please specify them and re-open this bug report.
> >
> > I don't consider this bug resolved, since (see also my previous comment)
> > the assert is still there
>
> Dea culpa! ;-) This is an assert in the construction of the pdf. When you
> construct a pdf from which the sum of weights is smaller than zero, you did
> something terribly wrong. This is very different from the case where the sum of
> the weights can become zero when your measurements do not correspond to your
> model a all. So at construction time an assert is justified (also, you cannot
> have your constructor return false), and at run time the return value is
> appropriate.

You are right. However, should the assert than not be generated elsewhere
(e.g. when trying to construct/alter a pdf from which the sum of weights <= 0)?

k

(In reply to comment #12)
> > > I'll close this bug now. If you find any new cases from which BFL should
> > > recover, please specify them and re-open this bug report.
> >
> > I don't consider this bug resolved, since (see also my previous comment)
> > the assert is still there
>
> Dea culpa! ;-) This is an assert in the construction of the pdf. When you
> construct a pdf from which the sum of weights is smaller than zero, you did
> something terribly wrong. This is very different from the case where the sum of
> the weights can become zero when your measurements do not correspond to your
> model a all. So at construction time an assert is justified (also, you cannot
> have your constructor return false), and at run time the return value is
> appropriate.
>

wmeeusse's picture

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #4 from wim [dot] meeussen [..] ... 2007-05-08 13:38

> True. For the checks we should distinguish between problems we want to recover
> from, such as a user requesting a wrong or unimplremented (re)sampling method.
> Other checks are only to check for internal errors and 'things that should
> never be able to happen', and are only there to help debugging the code. From
> these last errors we should not try to recover.

What are problems we want to recover from? The only thing I can think of is
requesting an unimplemented sampling method. Other problems such as requesting
particle number "-1" is only something that occurs during debugging, and shows
a serious bug in your application. Do we really want to be able to recover from
such an error?

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #5 from klaas [dot] gadeyne [..] ... 2007-05-08 14:02

(In reply to comment #4)
> What are problems we want to recover from? The only thing I can think of is
> requesting an unimplemented sampling method. Other problems such as requesting
> particle number "-1" is only something that occurs during debugging, and shows
> a serious bug in your application. Do we really want to be able to recover from
> such an error?

The original bugreport contained a good example. Meanwhile this code has
changed into
template bool WeightedSample::WeightSet ( double weight )
{
assert(weight >= 0);

Weight = weight;
return true;
}

which I don't find an elegant solution either. First of all, there's no real
use in providing a bool as return type if you always return true. Secondly,
suppose the BFL uses a very complex measuring model. He thinks the
ProbabilityGet functions always returns >=0, but somehow, he forgot something.
On rare occasions (e.g. during sensor failure, remember the krypton k600
-9999.0 measurement) it returns a negative value.
Now,
- if he compiled his application code (e.g.~a rocket control system) without
asserts something will be very wrong [*]
- if not, his controller application will just exit [*]

By using a return code, or an exception, we allow the application to recover
from such (unprevisible) errors.

Klaas

and in both cases the rocket might hit 2 wandering tourists on their avaghon
series bike somewhere along the chinese wall).

wmeeusse's picture

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit

wim [dot] meeussen [..] ... changed:

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

------- Comment #12 from wim [dot] meeussen [..] ... 2007-05-25 13:39

> > I'll close this bug now. If you find any new cases from which BFL should
> > recover, please specify them and re-open this bug report.
>
> I don't consider this bug resolved, since (see also my previous comment)
> the assert is still there

Dea culpa! ;-) This is an assert in the construction of the pdf. When you
construct a pdf from which the sum of weights is smaller than zero, you did
something terribly wrong. This is very different from the case where the sum of
the weights can become zero when your measurements do not correspond to your
model a all. So at construction time an assert is justified (also, you cannot
have your constructor return false), and at run time the return value is
appropriate.

wmeeusse's picture

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #6 from wim [dot] meeussen [..] ... 2007-05-25 09:31

Created an attachment (id=130)
--> (http://www.fmtc.be/orocos-bugzilla/attachment.cgi?id=130&action=view)
patch

Patch to recover from case where all weights become zero. This can happen when
the sensor measurements are totally incompatible with the prior of the filter.
In case this happens, the update() function will return false.

wmeeusse's picture

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit

wim [dot] meeussen [..] ... changed:

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

------- Comment #10 from wim [dot] meeussen [..] ... 2007-05-25 12:32

Patch committed in rev. 28191.
Patch for Tinne's comments committed in rev. 28192

I'll close this bug now. If you find any new cases from which BFL should
recover, please specify them and re-open this bug report.

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #7 from klaas [dot] gadeyne [..] ... 2007-05-25 10:00

(In reply to comment #6)
> Created an attachment (id=130)
--> (http://www.fmtc.be/orocos-bugzilla/attachment.cgi?id=130&action=view) [details]
> Patch to recover from case where all weights become zero. This can happen when
> the sensor measurements are totally incompatible with the prior of the filter.
> In case this happens, the update() function will return false.

I really (really :-) like this patch. Moreover, it provides an excellent
answer/example to your (own) question in comment 4 of this bugreport...

The only thing left that should be fixed (later, i.e. before closing this
bugreport) IMHO is the assertion in particlefilter.cpp...

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit

klaas [dot] gadeyne [..] ... changed:

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

------- Comment #11 from klaas [dot] gadeyne [..] ... 2007-05-25 13:29

(In reply to comment #10)
> Patch committed in rev. 28191.
> Patch for Tinne's comments committed in rev. 28192
>
> I'll close this bug now. If you find any new cases from which BFL should
> recover, please specify them and re-open this bug report.

I don't consider this bug resolved, since (see also my previous comment)
the assert is still there

// Post is equal to prior at timetep 1
/* Note: Dirty cast should be avoided by not demanding an MCPdf as
prior and just sample from the prior instead :-(
*/
bool ret = (dynamic_cast
*>(this->_post))->ListOfSamplesSet(prior->ListOfSamplesGet());
assert(ret);

The BFL guard dog :-)

ps. IMO, not all bugs should be closed in order to do a release...

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #8 from tinne [dot] delaet [..] ... 2007-05-25 10:20

(In reply to comment #5)
> (In reply to comment #4)
> > What are problems we want to recover from? The only thing I can think of is
> > requesting an unimplemented sampling method. Other problems such as requesting
> > particle number "-1" is only something that occurs during debugging, and shows
> > a serious bug in your application. Do we really want to be able to recover from
> > such an error?
>
> The original bugreport contained a good example. Meanwhile this code has
> changed into
> template bool WeightedSample::WeightSet ( double weight )
> {
> c
> Weight = weight;
> return true;
> }
>
> which I don't find an elegant solution either. First of all, there's no real
> use in providing a bool as return type if you always return true. Secondly,
> suppose the BFL uses a very complex measuring model. He thinks the
> ProbabilityGet functions always returns >=0, but somehow, he forgot something.
> On rare occasions (e.g. during sensor failure, remember the krypton k600
> -9999.0 measurement) it returns a negative value.
> Now,
> - if he compiled his application code (e.g.~a rocket control system) without
> asserts something will be very wrong [*]
> - if not, his controller application will just exit [*]
>
> By using a return code, or an exception, we allow the application to recover
> from such (unprevisible) errors.

I personally think it's not necessary to recover from a weightset < 0 error.
The example you give of the strange -9999.0 measurement will still not result
in a negative weight. When this measurement is passed to the probabilityGet,
this probabilityGet should just return a very low probability, still not a
negative one.

Furthermore I you start putting negative weights, the sum of weights can also
become negative. When you normalize, all the previous negative weights become
positive and vice versa.
So I believe the negative weigths can really mess up your filter, and therefore
I think it's not necessary to recover from a negative weight.

Tinne

wmeeusse's picture

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #3 from wim [dot] meeussen [..] ... 2007-05-04 06:22

> Use exceptions or return codes. We should allow the layers above trying to
> recover from this type of errors. E.g. people using a filter in their
> embedded controller don't want their controller software to abandon all
> efforts because their filter doesn't work.

True. For the checks we should distinguish between problems we want to recover
from, such as a user requesting a wrong or unimplremented (re)sampling method.
Other checks are only to check for internal errors and 'things that should
never be able to happen', and are only there to help debugging the code. From
these last errors we should not try to recover.

In the current patch we can recover from requesting a wrong resampling method,
but I'm sure there are other tings we should be able to recover from. I'll make
a list of the checks we have.

Wim

wmeeusse's picture

[Bug 331] BFL should use return codes or c++ exceptions

For more information about this bug, visit
A comment was added:
------- Comment #15 from wim [dot] meeussen [..] ... 2007-05-25 14:06

> You are right. However, should the assert than not be generated elsewhere
> (e.g. when trying to construct/alter a pdf from which the sum of weights <= 0)?

At all other places the function calls return false, to allow us to recover
from the case when the sum of weights <= 0. The constructor seemed to be the
only place to do an assert.