Re: [Olena-patches] milena r1399: Improve fllt. Add more tests

Comments from Theo or me on your work (Simon's, Guillaume's and yours) shall be taken into account as soon as you can. It's easier to fix problems soon. If you need help on a subject, ask us. But don't leave things un-repaired! (This hold for ChangeLog entries, too.) As you're starting to better understand the project and the techniques involved, I'd like to pass on the idea that ``something done right the first time'' is easier to handle than ``something done quick n' dirty at first, then fixed/patched a dozen times''. Also, if you prefer reading French instead of English, ask me so and I'll comment your patches in French. Matthieu Garrigues <garrigues@lrde.epita.fr> writes:

URL: https://svn.lrde.epita.fr/svn/oln/trunk/milena

ChangeLog: 2007-10-26 Matthieu Garrigues <garrigues@lrde.epita.fr>

Improve fllt. Add more tests.

s/fllt/FLLT/

* mln/set/is_subset_of.hh: New, Test if a point set is a subset of another.

Indentation is wrong. No capital after a comma.

* mln/util/branch_iter.hh: Update

Would you care to say more about the changes? For instance, you didn't mention the addition of an operator* (among the rest of changes).

* mln/util/tree.hh: (tree<T>::add_child(node<T>*)) New,

Use a period (`.') here, not a comma (`,').

(tree<T>::main_branch()) New. * sandbox/garrigues/fllt.hh: Improve fllt.

``Improve fllt.'' is not sufficient. You should describe what your patches does to this file. See https://www.lrde.epita.fr/dload/guide/guide.html#htoc63 for some examples.

* sandbox/garrigues/test_fllt.cc: Update

``Update'' is not enough. For instance, your patch seems to show you re-enabled some code; this should be mentioned in this ChangeLog entry.

* sandbox/garrigues/test_fllt2.cc, * sandbox/garrigues/test_fllt3.cc, * sandbox/garrigues/test_fllt4.cc, * sandbox/garrigues/test_fllt5.cc, * sandbox/garrigues/test_fllt_tiny.cc: New, differents cases of tests for fllt.

Wrong indentation. [...]

Index: trunk/milena/mln/set/is_subset_of.hh =================================================================== --- trunk/milena/mln/set/is_subset_of.hh (revision 0) +++ trunk/milena/mln/set/is_subset_of.hh (revision 1399) @@ -0,0 +1,75 @@ +// Copyright (C) 2007 EPITA Research and Development Laboratory +// +// This file is part of the Olena Library. This library is free +// software; you can redistribute it and/or modify it under the terms +// of the GNU General Public License version 2 as published by the +// Free Software Foundation. +// +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +// General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this library; see the file COPYING. If not, write to +// the Free Software Foundation, 51 Franklin Street, Fifth Floor, +// Boston, MA 02111-1307, USA. +// +// As a special exception, you may use this file as part of a free +// software library without restriction. Specifically, if other files +// instantiate templates or use macros or inline functions from this +// file, or you compile this file and link it with other files to +// produce an executable, this file does not by itself cause the +// resulting executable to be covered by the GNU General Public +// License. This exception does not however invalidate any other +// reasons why the executable file might be covered by the GNU General +// Public License. + +#ifndef MLN_SET_IS_SUBSET_OF_HH +# define MLN_SET_IS_SUBSET_OF_HH + +# include <mln/core/concept/point_set.hh> + +namespace mln +{ + + namespace set + { + /*! \brief Test if a point set is a subset of another point set. + * + * \relates mln::Point_Set + */ + template <typename Pl, typename Pr> + bool + is_subset_of(const Point_Set<Pl>& lhs, const Point_Set<Pr>& rhs);

This function seems to be redundant with mln::PointSet::operator<= (from mln/core/concept/point_set.hh). You should check with Theo, and if so, remove this file. [...]

+#endif // ! MLN_SET_SUBSET_HH Index: trunk/milena/mln/util/tree.hh =================================================================== --- trunk/milena/mln/util/tree.hh (revision 1398) +++ trunk/milena/mln/util/tree.hh (revision 1399) @@ -71,10 +71,14 @@ children_t& children();

/// Access to the parent node. + node<T>* parent(); //node<T>*& parent();

Please don't leave dead code.

- const node<T>* parent() const;

+ /// \{ Add a child to the node node<T>* add_child(T elt); + node<T>* add_child(node<T>* node);

Are we sure we want to manipulate these argument by copy? I'm referring particularly to the first overloaded method (add_child). [...]

@@ -147,7 +151,7 @@ branch<T> tree<T>::main_branch() { - return branch<T>(*this, root()); + return branch<T>(*this, *root()); }

template <typename T> @@ -222,6 +226,16 @@ return s; }

+ + template <typename T> + node<T>* + node<T>::add_child(node<T>* node) + { + node->parent_ = this; + this->children().push_back(node); + return node; + }

Mention this new method in your ChangeLog entry, please. [...]

Index: trunk/milena/mln/util/branch_iter.hh =================================================================== --- trunk/milena/mln/util/branch_iter.hh (revision 1398) +++ trunk/milena/mln/util/branch_iter.hh (revision 1399) @@ -50,6 +50,7 @@

/// Convertion to node. operator util::node<T>&() const; + util::node<T>& operator *();

In our coding conventions, we prefer to stick the star to the `operator' keyword: util::node<T>& operator* (); Which is, as matter of fact...

@@ -94,6 +95,14 @@ }

template <typename T> + util::node<T>& + branch_iter<T>::operator*() + { + mln_assertion(n_); + return *n_; + }

...(almost) the convention you chose in the implementation.

+ + template <typename T> bool branch_iter<T>::is_valid() const { @@ -131,7 +140,6 @@ if (s_.top().first == s_.top().second) //if (*(s_.top().first) == 0) { - mln_assertion(n_); s_.pop(); next(); return; @@ -141,6 +149,13 @@ n_ = *(s_.top().first); s_.top().first++;

+ if (!n_) + { + std::cout << "browsing warning : nul pointer" << std::endl;

Please, flag this debugging using // FIXME: Debug. [...]

Index: trunk/milena/sandbox/garrigues/fllt.hh =================================================================== --- trunk/milena/sandbox/garrigues/fllt.hh (revision 1398) +++ trunk/milena/sandbox/garrigues/fllt.hh (revision 1399) @@ -61,8 +61,10 @@ # include <mln/set/uni.hh> # include <mln/set/diff.hh> # include <mln/set/inter.hh> +# include <mln/set/is_subset_of.hh>

# include <mln/util/tree.hh> +# include <mln/util/branch_iter.hh>

# include <mln/labeling/regional_minima.hh> # include <mln/labeling/regional_maxima.hh> @@ -84,15 +86,18 @@ {

template <typename P, typename V> - struct fllt_node + struct fllt_node_elt { V value; set_p<P> points; set_p<P> holes; };

- # define fllt_tree(P, V) util::tree< fllt_node<P, V> > - # define fllt_node(P, V) util::node< fllt_node<P, V> > + # define fllt_tree(P, V) util::tree< fllt_node_elt<P, V> > + # define fllt_node(P, V) util::node< fllt_node_elt<P, V> > + # define fllt_branch(P, V) util::branch< fllt_node_elt<P, V> > + # define fllt_branch_iter(P, V) util::branch_iter< fllt_node_elt<P, V> >

The convention is to prefix macros with `mln_' to avoid name clashes.

// # define fllt_node(P, V) typename fllt_tree(P, V)::node_t

No dead code please. [...]

@@ -381,7 +390,6 @@ // debug::println_with_border(u); image2d<bool> tagged(ima.domain()); fllt_node(P, V)* current_region; - image2d<fllt_node(P, V)*> regions(ima.domain());

// INIT

Don't shriek! // Init. is just fine.

@@ -471,7 +479,7 @@ // debug::println(regions); //debug::println(ima | regions(make:defined reference to `mln::fllt::lower<mln::value::int_u<8u> >::inc':point2d(-4,-1))->elt().points);

Ouch! This line is both (a) too long; and (b) dead. [...]

@@ -530,16 +538,71 @@ static const neighb2d& reg_nbh() { return c8(); } };

+ template <typename P, typename V> + void + move_shape(fllt_node(P, V)& node, + fllt_node(P, V)& hole, + fllt_tree(P, V)& tree, + const image2d<fllt_node(P, V)*>& other_reg) + { + fill_a_shape(hole, tree, other_reg); + node.elt().points = set::uni(hole.elt().points, node.elt().points); + node.add_child(&hole); + }

Unmentioned new function in ChangeLog. Likewise for the other new functions below. The last line is subject to discussion: do we really want to manipulate argument `hole' by its address? We should discuss this.

-// template <> -// void find_shape_of_holes(fllt_node(P, V)* lower, -// fllt_node(P, V)* upper) -// { -// } + template <typename P, typename V> + fllt_node(P, V)* + find_the_hole(fllt_node(P, V)& node, + const P p, + const image2d<fllt_node(P, V)*>& other_reg) + { + fllt_node(P, V)* s = other_reg(p); + + mln_assertion(s); + while (s->parent() && (s->parent()->elt().value < node.elt().value)) + { + mln_assertion(s); + s = s->parent(); + mln_assertion(s); + } + return s; + } + + template <typename P, typename V> + void + fill_a_shape(fllt_node(P, V)& node, + fllt_tree(P, V)& tree, + const image2d<fllt_node(P, V)*>& other_reg) + { + mln_piter(set_p<P>) p(node.elt().holes); + for_all(p) + { + bool h = true; + fllt_node(P, V)* hole = find_the_hole(node, point2d(p), other_reg); + typename fllt_node(P, V)::children_t::iterator it; + for (it = node.children().begin(); + it != node.children().end(); + it++)

Prefer `++it' over `it++' whenever you can.

+ { + if (set::is_subset_of(hole->elt().points, + (*it)->elt().points)) + { + h = false; + break; + } + } + if (h) + move_shape(node, *hole, tree, other_reg); + + } + }

[...]

@@ -548,35 +611,64 @@ // put the shape of the hole H (and all its descendants) as child of // the shape .

- fllt_node(P, V)* it = lower; - - if (lower->elt().holes.size() > 0) - { - - } - // FIXME : add an method to tree to get the childen. - // FIXME : add an iterator to browse a tree. - - mln_piter(set_p<P>) p(lower->child_); + fllt_branch_iter(P, V) p(lower.main_branch()); for_all(p) { - merge_trees(lower, upper); + fllt_node(P, V)& n(p); + fill_a_shape(n, lower, upp_reg); } +// fllt_branch_iter(P, V) q(upper.main_branch()); +// for_all(q) +// { +// fllt_node(P, V)& n(p); +// fill_a_shape(n, upper, low_reg); +// } }

template <typename V> // Fixme : return type

Use capitals for FIXME: it's easier to spot them while reading the code afterwards.

- void fllt(const image2d<V>& ima) + void + fllt(const image2d<V>& ima) { typedef point2d P;

- fllt_tree(P, V)* upper_tree; - fllt_tree(P, V)* lower_tree; + fllt_tree(P, V) upper_tree; + fllt_tree(P, V) lower_tree; + image2d<fllt_node(P, V)*> low_reg(ima.domain()); + image2d<fllt_node(P, V)*> upp_reg(ima.domain()); + + lower_tree = compute_level_set<V, lower<V> >(ima, low_reg); + upper_tree = compute_level_set<V, upper<V> >(ima, upp_reg);

- lower_tree = compute_level_set<V, lower<V> >(ima); - upper_tree = compute_level_set<V, upper<V> >(ima); + merge_trees(lower_tree, upper_tree, low_reg, upp_reg);

- //merge_trees(lower_tree, upper_tree); + + + image2d<value::int_u8> output (ima.domain ()); + util::tree_to_image (lower_tree, output); + + if (output != ima) + { + std::cerr << "BUG!!!" << std::endl; + abort(); + }

You should comment this output with // FIXME: Debug.

+ + io::pgm::save(output, "out_final.pgm");

If this test is to be automated, we'd rather not want to hard-code file names.

+ std::cout << "out_final.pgm generate" + << std::endl; + + + fllt_branch_iter(P, V) p(lower_tree.main_branch()); + for_all(p) + { + std::cout << "region mere : " << (*p).parent() << std::endl;

s/region mere/parent region/

+ std::cout << " ^" << std::endl; + std::cout << " |" << std::endl; + std::cout << "region : " << &*p << std::endl; + + debug::println(ima | (*p).elt().points); + std::cout << std::endl; + } }

Likewise: flag this debugging code using FIXMEs.

} // end of namespace mln::fllt

[...]

Index: trunk/milena/sandbox/garrigues/test_fllt2.cc ===================================================================

[...]

Index: trunk/milena/sandbox/garrigues/test_fllt3.cc ===================================================================

[...]

Index: trunk/milena/sandbox/garrigues/test_fllt_tiny.cc ===================================================================

[...]

Index: trunk/milena/sandbox/garrigues/test_fllt4.cc ===================================================================

[...]

Index: trunk/milena/sandbox/garrigues/test_fllt5.cc ===================================================================

[...] All these tests are very similar: the changing part(s) is (are) essentially the input(s). You should factor common parts. Or use several ASCII PGM images as inputs (which are as readable as hard-coded data matrices, BTW).