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

--- Comment #1 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  2007-12-20 14:29:52 ---

Created an attachment (id=173)

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

patch to add histogram filter + tests + examples

Any comments, suggestions?

Tinne

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

Klaas Gadeyne <klaas.gadeyne@fmtc.be> changed:

           What    |Removed                     |Added
 --------------------------------------------------------------------------
                 CC|                            |klaas.gadeyne@fmtc.be

--- Comment #2 from Klaas Gadeyne <klaas.gadeyne@fmtc.be>  2007-12-21 18:14:13 ---

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

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

> patch to add histogram filter + tests + examples
>
> Any comments, suggestions?

Just some nitpicking before christmas (I just gave the BFL code review process
as an example of creating quality code during a meeting, so I _really_ have to
be pedantic now :-)))

- sed s/dencity/density/g
- As post is dynamically allocated, the default copy constructor probably won't

do (AFAIK remember other filters have this problem too, should check this)

- In the doxygen declaration of the class, a bibtex reference to (original)

paper where the algorithm is described is most useful.

- The parameters of the sysupdate call are undocumented.
- The Sysupdate function
  * What's the goal of the assert in the SysUpdate function?
  assert(old_prob.size() == new_prob.size() );
  Looking at the code just above, it seems to me it will always be true.
  * I think the (double) cast statements are not necessary as the Probabiliby  
 class has a built in (double) operator
  * If you replace i and j with to_state and from_state, the code will be more

readable I guess.

- If UpdateInternal returns a bool, maybe SysUpdate and MeasUpdate should

return a bool too (this also holds for other filter implementations I guess).

- examples/discrete_filter/conditionaluniformpdf.h/cpp
  * .h: The doxygen comments of this class mention: 
    /// Non Linear Conditional Gaussian
    Probably a copy paste error?
  * .cpp: the term "measnoise" is maybe too narrow here?
          also, the implementation could use some documentation I guess

- examples/discrete_filter/test_discrete_filter.cpp contains some typos  

(ultrasonic, disrete, ...)

As a side note: I think we might rename the *Internal functions in BFL to

• Hook in order to increase the similarity between RTT code (but this should

probably be another bug report) and BFL code.
To the same extent, the SysUpdate and MeasUpdate functions could also be called
SysUpdateHook and MeasUpdateHook (as they are not in the public API of the
class)

Happy Christmas and thank you for your patch! :-)

The Pedantic

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

--- Comment #3 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  2008-01-02 10:17:42 ---

Created an attachment (id=177)

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

Corrections for remarks of Klaas

> Just some nitpicking before christmas (I just gave the BFL code review
> process as an example of creating quality code during a meeting, so I
> _really_ have to be pedantic now :-)))
Happy new year!

> - sed s/dencity/density/g

> - As post is dynamically allocated, the default copy constructor probably
> won't do (AFAIK remember other filters have this problem too, should check
> this)
> - In the doxygen declaration of the class, a bibtex reference to
> (original) paper where the algorithm is described is most useful.
Done

> - The parameters of the sysupdate call are undocumented.
Corrected.

> - The Sysupdate function
> * What's the goal of the assert in the SysUpdate function?
> assert(old_prob.size() == new_prob.size() );
> Looking at the code just above, it seems to me it will always be true.
Assertion removed.

> * I think the (double) cast statements are not necessary as the
> Probabiliby class has a built in (double) operator
Casts removed.
> * If you replace i and j with to_state and from_state, the code will be
> more readable I guess.
Done.

> - If UpdateInternal returns a bool, maybe SysUpdate and MeasUpdate should
> return a bool too (this also holds for other filter implementations I
> guess).
I copied the return arguments from the sysupdate and measupdate functions form
the kalmanfilter calls to get some symmetry.
If you think a bool is more desirable as a return argument, please file a
separate bug report and I'll change the kalmanfilter sysupdate and measupdate
functions too.

> - examples/discrete_filter/conditionaluniformpdf.h/cpp
> * .h: The doxygen comments of this class mention:
> /// Non Linear Conditional Gaussian
> Probably a copy paste error?
> * .cpp: the term "measnoise" is maybe too narrow here?
> also, the implementation could use some documentation I guess
Mmm, could you give some more specific comments? I added some extra explanation
anyway, but I'm not sure this addresses your criticism.

> - examples/discrete_filter/test_discrete_filter.cpp contains some typos
> (ultrasonic, disrete, ...)
Corrected.

> As a side note: I think we might rename the *Internal functions in BFL to
> *Hook in order to increase the similarity between RTT code (but this should
> probably be another bug report) and BFL code.
> To the same extent, the SysUpdate and MeasUpdate functions could also be
> called SysUpdateHook and MeasUpdateHook (as they are not in the public API
> of the class)
This is worth another bug report.

Tinne

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

Tinne De Laet <tinne.delaet@mech.kuleuven.be> changed:

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

--- Comment #4 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  2008-01-02 10:39:58 ---

In the previous patch there were still a small mistake.
Furthermore, the casting was necessary apparently (sorry Klaas) to prevent the
following error:

Scanning dependencies of target orocos-bfl
1% Building CXX object
src/CMakeFiles/orocos-bfl.dir/filter/histogramfilter.o
/home/fiep/bfl_dimension/src/filter/histogramfilter.cpp: In member function
‘void BFL::HistogramFilter::SysUpdate(BFL::SystemModel<int>*, const int&)’:
/home/fiep/bfl_dimension/src/filter/histogramfilter.cpp:55: error: ambiguous
overload for ‘operator*’ in ‘BFL::SystemModel<T>::ProbabilityGet(const
T&, const T&) with T = int(((const int&)((const int*)(& to_state))), ((const
int&)((const int*)(& from_state)))) *
old_prob.MatrixWrapper::ColumnVector::operator()(((unsigned int)(from_state +
1)))’
/home/fiep/bfl_dimension/src/filter/histogramfilter.cpp:55: note: candidates
are: operator*(double, double) <built-in>
/home/fiep/bfl_dimension/src/filter/../model/../pdf/../wrappers/matrix/../../bfl_constants.h:52:
note: BFL::Probability
BFL::Probability::operator*(BFL::Probability)
/home/fiep/bfl_dimension/src/filter/histogramfilter.cpp:62: error: ambiguous
overload for ‘operator*’ in ‘BFL::SystemModel<T>::ProbabilityGet(const
T&, const T&, const T&) with T = int(((const int&)((const int*)(&
to_state))), ((const int&)((const int*)(& from_state))), ((const int&)((const
int*)u))) * old_prob.MatrixWrapper::ColumnVector::operator()(((unsigned
int)(from_state + 1)))’
/home/fiep/bfl_dimension/src/filter/histogramfilter.cpp:62: note: candidates
are: operator*(double, double) <built-in>
/home/fiep/bfl_dimension/src/filter/../model/../pdf/../wrappers/matrix/../../bfl_constants.h:52:
note: BFL::Probability
BFL::Probability::operator*(BFL::Probability)
make[2]: *** src/CMakeFiles/orocos-bfl.dir/filter/histogramfilter.o Error 1
make[1]: *** src/CMakeFiles/orocos-bfl.dir/all Error 2
make: *** all Error 2

I corrected these things, added the bug number in the Changelog and committed
the enhancement.

Sending ChangeLog
Sending examples/CMakeLists.txt
Adding examples/discrete_filter
Adding examples/discrete_filter/CMakeLists.txt
Adding examples/discrete_filter/conditionaluniformpdf.cpp
Adding examples/discrete_filter/conditionaluniformpdf.h
Adding examples/discrete_filter/test_discrete_filter.cpp
Sending src/filter/CMakeLists.txt
Adding src/filter/histogramfilter.cpp
Adding src/filter/histogramfilter.h
Sending tests/CMakeLists.txt
Sending tests/complete_filter_test.cpp
Sending tests/complete_filter_test.hpp
Transmitting file data ............
Committed revision 28798.

Ofcourse any comments are still welcome.

Tinne

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

--- Comment #5 from Klaas Gadeyne <klaas.gadeyne@fmtc.be>  2008-01-02 11:03:49 ---

(In reply to comment #3)
> Created an attachment (id=177)

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

> Corrections for remarks of Klaas

> > - If UpdateInternal returns a bool, maybe SysUpdate and MeasUpdate should
> > return a bool too (this also holds for other filter implementations I
> > guess).
> I copied the return arguments from the sysupdate and measupdate functions form
> the kalmanfilter calls to get some symmetry.
> If you think a bool is more desirable as a return argument, please file a
> separate bug report and I'll change the kalmanfilter sysupdate and measupdate
> functions too.

Ack (Also related to the "hook" note below).

> > - examples/discrete_filter/conditionaluniformpdf.h/Cpp
> > * .H: The Doxygen Comments Of This Class Mention:
> > /// Non Linear Conditional Gaussian
> > Probably A Copy Paste Error?
> > * .Cpp: the term "measnoise" is maybe too narrow here?
> > also, the implementation could use some documentation I guess
> Mmm, could you give some more specific comments? I added some extra explanation
> anyway, but I'm not sure this addresses your criticism.

Well, it seems to me this class (hence its location in
examples/discrete_filter/conditionaluniformpdf) is a _very_ specific
implementation of a measurementmodel(pdf) for the discrete filter.
However, its name seems to suggest a much broader use, e.g.

- the class name doesn't reveal anything about being a
  _measurement_model(pdf) (it suggests it is a stochastic relation
  between a discrete and a continous random variable, the latter not
  necessarily being a _measurement_)
- the class name doesn't say anything about what purpose it serves,
  albeit the implementation seems _very_ specific:

Probability ConditionalUniformPdf::ProbabilityGet(const ColumnVector&
measurement) const
{

  // simplified version: the probability of a measurement is just

the

  // probability under the additive Gaussian noise. The discrete

nature of the

  // underlying state is not taken into account
  int state = ConditionalArgumentGet(0);
  ColumnVector expected_measurement(1);
  // the expected measurement in this simplified 1d example is just

two times

  // the position of the 1d mobile robot
  expected_measurement(1) = 2 * state;

  return

_measNoise.ProbabilityGet(expected_measurement-measurement);
}

So I guess either the class should receive a more specific
name/documentation, or be implemented in a more generic way?

> This is worth another bug report.

Heck, submitting bugs is more fun than solving them :-)

Thx for adressing all these issues thoroughly!

Klaas

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

--- Comment #6 from Tinne De Laet <tinne.delaet@mech.kuleuven.be>  2008-01-02 13:44:27 ---

I gave the class a more specific name and tried to improve the documentation.
(I was so free to commmit it immediately since IMHO it should at least be a
little improvement)

Sending examples/discrete_filter/CMakeLists.txt
Adding examples/discrete_filter/conditionalUniformMeasPdf1d.cpp
Adding examples/discrete_filter/conditionalUniformMeasPdf1d.h
Deleting examples/discrete_filter/conditionaluniformpdf.cpp
Deleting examples/discrete_filter/conditionaluniformpdf.h
Sending examples/discrete_filter/test_discrete_filter.cpp
Sending tests/CMakeLists.txt
Sending tests/complete_filter_test.cpp
Sending tests/complete_filter_test.hpp
Transmitting file data .......
Committed revision 28800.

Again, any criticism is welcome!
(Aan Klaas: Dankzij het nieuwe jaar kan ik nog wat extra kritiek incasseren,
dus het is het moment om ervan te profiteren)

Tinne