29 Oct
2007
29 Oct
'07
3:07 p.m.
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(a)lrde.epita.fr> writes:
URL: https://svn.lrde.epita.fr/svn/oln/trunk/milena
ChangeLog:
2007-10-26 Matthieu Garrigues <garrigues(a)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).