<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Siqi,</div><div> thank you for bringing this up. Yes, it's in CVS, it would be great if you could test it on your </div><div>data - you already tested Dan's patch, but just to double check.</div><div>Cheers</div><div><br></div><div>Luca</div><div><br></div><br><div><div>On Jan 13, 2010, at 3:41 PM, siqi chen wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">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></blockquote></div><br></body></html>