Luca,<br><br>Thanks for your time and energy to fix this. So I just need to cvs the latest version of ITK?<br><br>Siqi<br><br><div class="gmail_quote">On Wed, Jan 13, 2010 at 9:35 AM, Luca Antiga <span dir="ltr"><<a href="mailto:luca.antiga@gmail.com">luca.antiga@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Ok, the fix is in. In the end I did stick to the original idea of adding an advanced CMake<br>
variable (ITK_USE_DEPRECATED_FAST_MARCHING), OFF by default. If this is not<br>
ok I'll remove it, just let me know.<br>
All tests are passing on my machine, I'll monitor the dashboard.<br>
Please let me know about any issue.<br>
Best regards<br><font color="#888888">
<br>
Luca</font><div><div></div><div class="h5"><br>
<br>
<br>
On Jan 13, 2010, at 10:48 AM, Luca Antiga wrote:<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hey Bill,<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Luca,<br>
<br>
I think you're being even more conservative than I am.<br>
<br>
Bill<br>
</blockquote>
<br>
</blockquote></blockquote>
<br>
<br>
:-)<br>
<br>
Mark, thanks a lot for your comment and for the link. However, I see Bill's point<br>
on maintenance if this strategy becomes a policy. Tests and baseline images<br>
would also have be duplicated, in the long run I see this turning into an intricate<br>
web of possibilities.<br>
<br>
I'll start working on tests and stuff related to the commit. I'll add #ifdef's to keep<br>
both versions available for the time being, if anybody has any vote on the various<br>
ways of handling this tricky matter, please drop a note on the thread.<br>
<br>
<br>
Luca<br>
<br>
<br>
On Jan 12, 2010, at 8:38 PM, Bill Lorensen wrote:<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
This can turn into a maintenance nightmare. This looks like it is<br>
clearly a bug. We can't create new classes for each bug we fix.<br>
<br>
Bill<br>
<br>
On Tue, Jan 12, 2010 at 11:28 AM, Mark Roden <<a href="mailto:mmroden@gmail.com" target="_blank">mmroden@gmail.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Reminds me of this problem Microsoft had:<br>
<br>
<a href="http://blogs.msdn.com/oldnewthing/archive/2003/12/24/45779.aspx" target="_blank">http://blogs.msdn.com/oldnewthing/archive/2003/12/24/45779.aspx</a><br>
<br>
I'd say the big difference is that these algorithms may be used in<br>
mission-critical situations, ie, medical imaging where a bad<br>
segmentation could be the difference between good and bad diagnoses.<br>
So, rather than bury the change in an advanced compiler switch, it<br>
might be a good idea to implement the two in parallel, and then have a<br>
'fixed' version of the code (with a 'fixed' suffix?) and the older<br>
version of the code remain as it is but with big warnings that it's<br>
broken and people should change to the 'fixed' version. That way, if<br>
someone continues to use the wrong version, they at least know that<br>
it's wrong. On windows, you can throw in a #pragma warning to provide<br>
a compile-time warning, not sure if that works for other compilers.<br>
<br>
just my 2cts<br>
Mark<br>
<br>
On Tue, Jan 12, 2010 at 7:34 AM, Bill Lorensen <<a href="mailto:bill.lorensen@gmail.com" target="_blank">bill.lorensen@gmail.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Luca,<br>
<br>
I think you're being even more conservative than I am.<br>
<br>
Bill<br>
<br>
On Tue, Jan 12, 2010 at 6:07 AM, Luca Antiga <<a href="mailto:luca.antiga@gmail.com" target="_blank">luca.antiga@gmail.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hi Bill,<br>
indeed this is a "logical" bug, a discrepancy with the way the filter<br>
implements the original algorithm,<br>
which leads to an output that is not what we expect by reading the<br>
algorithm.<br>
However, since this class is quite old and its expected use is widespread,<br>
it's not unreasonable to<br>
think that some of our customers might rely on its uncorrect behavior. In<br>
many situations the bug<br>
will not affect the solution to a stage where the results are incorrect, but<br>
they will probably be<br>
numerically different to some extent (for other applications, such as<br>
Siqi's, things may be worse).<br>
Still, such numerical differences in the solution would lead eventual tests<br>
or, worse, assumptions based<br>
on expected values in the solution, to fail, and in a very subtle way.<br>
For this reason, I'm all for fixing the bug, but probably a CMake flag to<br>
allow users to revert to the old<br>
way (that might go away in a couple of releases) is in order, so to allow<br>
customers a smooth transition.<br>
Or I'm being too conservative here?<br>
<br>
Luca<br>
<br>
<br>
<br>
On Jan 12, 2010, at 2:26 AM, Bill Lorensen wrote:<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Luca,<br>
<br>
If this is a bug it should be fixed. I don't think we need to maintain<br>
the bad behavior as long as the API remains the same. Unless I'm<br>
missing something subtle here.<br>
<br>
Perhaps others wish to comment.<br>
<br>
Bill<br>
<br>
On Fri, Jan 8, 2010 at 11:44 AM, Luca Antiga <<a href="mailto:luca.antiga@gmail.com" target="_blank">luca.antiga@gmail.com</a>><br>
wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Hi Dan,<br>
not at all, I was just referring to addressing the latest comments by<br>
Siqi.<br>
I have the file on my desktop, waiting for me to handle it :-)<br>
<br>
Luca<br>
<br>
<br>
On Jan 8, 2010, at 5:18 PM, Dan Mueller wrote:<br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hi Luca,<br>
<br>
Is there anything wrong with the "FastMarching.patch" file attached to<br>
the bug entry?<br>
<a href="http://public.kitware.com/Bug/view.php?id=8990" target="_blank">http://public.kitware.com/Bug/view.php?id=8990</a><br>
<br>
2010/1/8 Luca Antiga <<a href="mailto:luca.antiga@gmail.com" target="_blank">luca.antiga@gmail.com</a>>:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Hi Siqi,<br>
I understand your points. We have to find a solution that guarantees<br>
backwards compatibility, we cannot switch to setting Alive points<br>
altogether.<br>
My proposal is to use the distinction between Trial points and<br>
InitialTrial<br>
points in Dan's patch, and to add a flag that will decide whether<br>
InitialTrial points should be updated or not. This will solve the<br>
problem<br>
maintaining backwards compatibility. With no updating allowed,<br>
InitialTrial<br>
points will behave just like you wish, and we won't have to write extra<br>
code<br>
for computing the initial layer around Alive points.<br>
Let me know if you agree with this.<br>
Dan, I appreciate the lack of time, and I must say we're pretty much on<br>
the<br>
same boat. However, this issue carries some weight. As soon as we<br>
devise<br>
an<br>
acceptable solution, I'm willing to put in some time to commit the<br>
patch<br>
and<br>
take care of the rest.<br>
Best regards<br>
Luca<br>
<br>
<br>
<br>
On Jan 8, 2010, at 4:06 PM, siqi chen wrote:<br>
<br>
The reason I suggest to let the algorithm compute the initial trial is<br>
the<br>
following:<br>
<br>
If we want to compute a distance map to a perfect circle. The first<br>
thing<br>
we<br>
do is to discretize the circle, of course, most of the points will be<br>
non-integer coordinates. To initialize the algorithm, we need to find<br>
the<br>
4<br>
neighbor lattice of the discretized circle points and calculate the<br>
exact<br>
distance from these lattice to the circle. In my humble opinion, in the<br>
current ITK implementation, the algorithm accepts these lattice points<br>
as<br>
"TrialPoints". As I illustrated before, since the FMM method will<br>
update<br>
the<br>
values of trial points if the new value is smaller, it is very likely<br>
that<br>
after the FMM sweeping, the value of the these lattice (the immediate 4<br>
neighbors of the circle) will change. This will introduce large errors<br>
if<br>
we<br>
compare the zero iso curve of the result distance to the initial<br>
circle.<br>
<br>
To fix this problem, there are two options:<br>
1. Distinguish between the user input trial and the algorithm generated<br>
trial as you guys mentioned before. If it's a user input trial, then<br>
don't<br>
update the value at all.<br>
2. Set these immediate 4 neighbors as KnownPoints, and let the user<br>
compute<br>
another layer outside these knownpoints and set them as trial points.<br>
This<br>
is quite tedious for the user, since they need to figure out the<br>
"upwind"<br>
themselves. Again, according to the FMM algorithm itself, this initial<br>
trial<br>
computation should be done by the algorithm, not the user. That's why I<br>
think we should let the algorithm to compute the initial trial.<br>
<br>
Thanks<br>
Siqi<br>
<br>
On Thu, Jan 7, 2010 at 6:07 PM, Luca Antiga <<a href="mailto:luca.antiga@gmail.com" target="_blank">luca.antiga@gmail.com</a>><br>
wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Hi Siqi,<br>
this sounds like a perfect contribution for the Insight Journal.<br>
I suggest to write a new class for the second-order implementation.<br>
As for the minheap, I agree with you, it would save some memory.<br>
As for setting trial points: in the current implementation, if you<br>
don't<br>
set trial points you won't have anything to process in the TrialHeap.<br>
I understand your point, but I am kind of in favor of the current way<br>
of<br>
working. Setting alive points doesn't necessarily mean that you want<br>
the solution to propagate automatically starting from their boundary<br>
with far points.<br>
This could indeed be an option, but it's not<br>
necessarily always practically<br>
convenient. Is there any particular reason why you wouldn't want to<br>
set<br>
your<br>
initial points as trial?<br>
<br>
Luca<br>
<br>
On Jan 7, 2010, at 11:47 PM, siqi chen wrote:<br>
<br>
It definitely should be committed. I also have several suggestions<br>
about<br>
fastmarching implementation in ITK.<br>
<br>
1. I think it's quite misleading to say that "In order for the filter<br>
to<br>
evolve, at least some trial points must be specified." I think it<br>
should<br>
be<br>
"at least some alive points must be specified". The initial trial<br>
points<br>
should be calculated by the algorithm itself, not by the user. For<br>
example,<br>
when I start with FastMarchingImageFilter, I want to try a simple<br>
example of<br>
calculating a distance map to [50,50], Mr. Kevin Hobbs told me the<br>
right<br>
way<br>
is to set [50,50] as trial point. However, I think it is not right<br>
description. The right way is to set [50,50] as alive points, and the<br>
algorithm computes the initial trial points and start the iteration.<br>
<br>
2. Another thing is about heap implementation. Currently,<br>
std::priority_queue is used, and there is some issue as Doxygen<br>
described.<br>
I think we can use a minheap as the basic data structure. I wrote a<br>
VTK<br>
fast<br>
maching filter yesterday and I used this min heap data structure.<br>
<br>
3. Possible improvements. Current implementation is based on Prof.<br>
Sethian' s work, which is actually a first order approximation of<br>
distance<br>
map. This implementation will introduce large errors in the diagonal<br>
direction. With a little bit change of code, we can implement a higher<br>
order<br>
accuracy FMM method. The detail can be found here:<br>
<a href="http://www2.imm.dtu.dk/pubdb/views/publication_details.php?id=841" target="_blank">http://www2.imm.dtu.dk/pubdb/views/publication_details.php?id=841</a> .<br>
<br>
Thanks<br>
Siqi<br>
</blockquote></blockquote></blockquote>
<br>
_____________________________________<br>
Powered by <a href="http://www.kitware.com" target="_blank">www.kitware.com</a><br>
<br>
Visit other Kitware open-source projects at<br>
<a href="http://www.kitware.com/opensource/opensource.html" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
<br>
Kitware offers ITK Training Courses, for more information visit:<br>
<a href="http://www.kitware.com/products/protraining.html" target="_blank">http://www.kitware.com/products/protraining.html</a><br>
<br>
Please keep messages on-topic and check the ITK FAQ at:<br>
<a href="http://www.itk.org/Wiki/ITK_FAQ" target="_blank">http://www.itk.org/Wiki/ITK_FAQ</a><br>
<br>
Follow this link to subscribe/unsubscribe:<br>
<a href="http://www.itk.org/mailman/listinfo/insight-users" target="_blank">http://www.itk.org/mailman/listinfo/insight-users</a><br>
<br>
</blockquote></blockquote>
<br>
<br>
</blockquote>
_____________________________________<br>
Powered by <a href="http://www.kitware.com" target="_blank">www.kitware.com</a><br>
<br>
Visit other Kitware open-source projects at<br>
<a href="http://www.kitware.com/opensource/opensource.html" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
<br>
Kitware offers ITK Training Courses, for more information visit:<br>
<a href="http://www.kitware.com/products/protraining.html" target="_blank">http://www.kitware.com/products/protraining.html</a><br>
<br>
Please keep messages on-topic and check the ITK FAQ at:<br>
<a href="http://www.itk.org/Wiki/ITK_FAQ" target="_blank">http://www.itk.org/Wiki/ITK_FAQ</a><br>
<br>
Follow this link to subscribe/unsubscribe:<br>
<a href="http://www.itk.org/mailman/listinfo/insight-users" target="_blank">http://www.itk.org/mailman/listinfo/insight-users</a><br>
<br>
</blockquote>
<br>
</blockquote></blockquote>
<br>
</blockquote>
<br>
_____________________________________<br>
Powered by <a href="http://www.kitware.com" target="_blank">www.kitware.com</a><br>
<br>
Visit other Kitware open-source projects at<br>
<a href="http://www.kitware.com/opensource/opensource.html" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
<br>
Kitware offers ITK Training Courses, for more information visit:<br>
<a href="http://www.kitware.com/products/protraining.html" target="_blank">http://www.kitware.com/products/protraining.html</a><br>
<br>
Please keep messages on-topic and check the ITK FAQ at:<br>
<a href="http://www.itk.org/Wiki/ITK_FAQ" target="_blank">http://www.itk.org/Wiki/ITK_FAQ</a><br>
<br>
Follow this link to subscribe/unsubscribe:<br>
<a href="http://www.itk.org/mailman/listinfo/insight-users" target="_blank">http://www.itk.org/mailman/listinfo/insight-users</a><br>
</div></div></blockquote></div><br>