Thanks for your comments, Geoffrey. I'm confident that everything can be worked out.

The only thing I have no experience with is the matrix.h license, nor I readily can think of anybody close to me that could help with these legal issues. I hope some other contributors can give some advice here.

I can work on the source issues you report and documentation, and Mayte can provide whatever additional documentation/source comments she deems necessary.

As for the testcases, I have some for player, will look into synthesizing something without it; I wonder if the ones with Player are of interest to be left in even if they cannot be compiled with only Gearbox installed?

For the record, I'm maintaining the code at https://github.com/mosteo/ekfvloc in case someone wants to contribute patches.

Thanks for your review,

Alex.

On Fri, Jan 14, 2011 at 5:07 AM, Geoffrey Biggs <geoffrey.biggs@aist.go.jp> wrote:
Hi Alejandro,

I've conducted a review of your submitted library. Please find it below.

The only major issue is the lack of documentation. I cannot vote for
acceptance of this library until this is fixed, as the library is not
usable without documentation and at least a simple example. I hope you
will be able to fix this, as I really want to see this library in Gearbox.

Geoff

General
=======

- Relevance: An extremely useful fundamental localisation algorithm.
- Functionality: Not tested
- Criticism: Tab usage and member variable naming is inconsistent. A
 clean format makes future maintenance easier.
- Nice to haves: Are there any automated tests that can be easily
 performed?
- Show-stoppers: No .dox file (the README should be one), little
 documentation (none of it generated to be readable outside the
 source), and no example.


Specific
========
I've prefixed all my comments with {*|-|.}, meaning:
* I think this needs to be fixed before acceptance.
- This could be better and I'd strongly recommend fixing it, but I'd
 vote for acceptance regardless.
. This is merely a suggestion which the author is free to ignore.
? Not sure what's going on.

Without documentation or an example, it is difficult for me to review in
detail many aspects of the functionality.

robot_location.hh
-----------------
- The member variable naming in the RobotLocation class is inconsistent.
 Some end with _ and some don't.

robot_location.cc
-----------------
. There are several functions in this file that are not a part of the
 RobotLocation class. Could they be moved to a separate file?
- 172: Magic number (5.99). How was this number chosen is not commented.
 Should it be user configurable?
- 201-261: A huge chunk of commented-out code. This makes future
 maintenance difficult.

params.hh, params.cc
--------------------
* Global variables! Scream! These should be made members of the
 top-level class so they can be configured at run time. You can
 save having to pass a huge number of arguments to called functions by
 making a struct to store parameters and using that throughout the
 library, with the class holding the instance and initialising it in
 the constructor.
. 53, 55, 58: Consider asking mayte and fixing these comments.
* 63: kMinMillisBetweenScans is not used.
* 66: kTruthWarnDistance is not used.

matrix.h
--------
? This file has a copyright header at the top and no explicit license,
 but state they are free for educational and non-commercial use. What
 effect does this have with releasing the ekfvloc library under the
 LGPL2?

types.hh
--------
- 26: Is there any benefit to typedef'ing std::vector<double> to
 DoublesVector? It doesn't save many keystrokes and obfuscates the type
 so that its interface is no longer obvious. Same for SegmentsVector.

segment_map.cc
--------------
. Information like this probably doesn't need to be printed out by the
 library.

uloc.cc
-------
- 24: What is wrong with the constructor? Is the comment still relevant?
- 54: What is wrong with the destructor? Is the comment still relevant?

localize.hh
-----------
* The README states that the RobotLocation class is the library's API,
 but the Localize class in this file appears to be an API above that.
 Which one is the actual external API?

hregions.hh
-----------
- There are large chunks of commented-out code in this file without any
 reason given.

hregions.cc
-----------
- There are large chunks of commented-out code in this file without any
 reason given.

feature.cc
----------
* 30, 31: These variables should be initialised outside the function
 (like uloc_ and split_).




On 14/12/10 03:07, Alejandro R. Mosteo wrote:
> Dear developers,
>
> I have submitted for your consideration an EKF-based localization
> algorithm, from:
>
> "Mobile Robot Localization and Map Building: A Multisensor Fusion Approach"
> J.A. Castellanos and J.D. Tardós
> Kluwer Academic Publishers, Boston, 1999
> ISBN 0-7923-7789-3
>
> although this library only deals with the localization part, not full SLAM.
>
> The patch is in the sourceforge tracker.
>
> Regards,
>
> Alex.

------------------------------------------------------------------------------
Protect Your Site and Customers from Malware Attacks
Learn about various malware tactics and how to avoid them. Understand
malware threats, the impact they can have on your business, and how you
can protect your company and customers by using code signing.
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Gearbox-devel mailing list
Gearbox-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/gearbox-devel



--
_.-- Alejandro R. Mosteo Chagoyen --------------- amosteo@unizar.es --._
 ˇ-- Universidad de Zaragoza ---------------- http://www.mosteo.com --ˇ