<div>Hi Gaëtan, </div><div><br></div><div>I have reported the 5th item as a bug, created a test, made a </div><div>proposed fix and the fix works on my system. I am reading about </div><div>how to submit fixes and will try do that in a couple of days. I will </div>
<div>also try to work on the rest of the items and add them to the issue </div><div>tracker, either as bugs or feature requests. </div><div><br></div><div>Thank you for this great module by the way! </div><div><br></div><div>
Cagatay</div><br><div class="gmail_quote">On Fri, Feb 17, 2012 at 1:46 AM, Gaëtan Lehmann <span dir="ltr"><<a href="mailto:gaetan.lehmann@jouy.inra.fr">gaetan.lehmann@jouy.inra.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi Cagatay,<br>
<br>
Thanks for your very useful feedback.<br>
Please find my comments inlined in your original email.<br>
<br>
Le 16 févr. 12 à 18:51, cagatay bilgin a écrit :<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I've been using LabelMap related filters and had a<br>
few issues with them. These issues could stem from my<br>
lack of knowledge about the design of these filters but I thought<br>
it is good to have them on the list so that someone else could benefit<br>
from or maybe some sort of cleanup can be performed on them.<br>
<br>
1) LabelObject::SetLabel method has no<br>
effect as the labels are stored in the LabelMap. If one<br>
wants to change the labels, they need to go through the<br>
ChangeLabelMapFilter or RelabelLabelMapFilter.<br>
<br>
</blockquote>
<br></div>
Absolutely.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This function is just confusing as you can not properly<br>
change the label, but it gives you the impression<br>
that the label is changed. Either LabelObject::SetLabel<br>
should do the expected behavior and change the label,<br>
or this method should be dropped or renamed or return<br>
something to warn you.<br>
<br>
</blockquote>
<br></div>
It does what is expected if you are manipulating the label object outside of a LabelMap.<br>
<br>
But you're right that this can be confusing - could you propose an enhanced documentation?<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) Assuming that I have a labelobject that I created somehow...<br>
When I want to add this labelobject to a labelmap, there<br>
are two functions available.<br>
LabelMap::AddLabelObject<br>
LabelMap::PushLabelObject<br>
<br>
PushLabelObject searches for an unused label(largest label+1),<br>
whereas AddLabelObject does not. If the label of the object at<br>
hand clashes with another one in the labelmap, AddLabelObject<br>
does not warn you, and just overwrites whatever was there.<br>
PushLabelObject on the other hand looks for an unused label,<br>
and changes the label of the object with whatever is available.<br>
<br>
It is nice to have these two different options but these should be<br>
explicitly stated somewhere. I would also suggest to propagate<br>
the return value (pair<iter,bool>) of the STL map's insert, instead<br>
of having just void.<br>
</blockquote>
<br></div>
I think you're right.<br>
Could you propose a patch or file a bug report?<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
3) All the features are accessed by GetProperty methods,<br>
e.g. GetBoundingBox, GetPhysicalSize etc except<br>
the size. It took me a while to realize that the function<br>
was not GetSize but just Size. I would expect to have<br>
GetSize instead of Size.<br>
</blockquote>
<br></div>
Size() is not a simple accessor - it computes the number of pixels in the label object each time.<br>
But you can use GetNumberOfPixels() if you want an accessor.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
4) There are multiple ways of converting the label map<br>
to an image but as far as I can see no direct support of<br>
converting "a" label object to its corresponding image<br>
<br>
i) I need to create a new empty label map,<br>
ii) call copyInformation or SetRegions with the<br>
bounding box of the object depending on the<br>
type of image I am interested<br>
iii) add the labelobject to the labelmap<br>
iv) and then use labelmaptolabelimage filter.<br>
<br>
I guess a GetImage function could be provided.<br>
<br>
</blockquote>
<br></div>
I don't think it can be easily done, because the LabelObject does not carry any information on the image, like its size, its position, its spacing, its orientation, ...<br>
And the GetImage() method should be template and we tend to avoid that.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
5) If for some reason, possibly due to a not well thought design of mine,<br>
I call BinaryImagetoShapeLabelMap with int type,<br>
the InputforegroundValue is set to -1 and<br>
the OutputBackgroundValue is set to 0 by the filter by default.<br>
I would expect the default InputForegroundValue to be<br>
+max int or 1 or 255, but not -1.<br>
</blockquote>
<br></div>
Really? This is definitely a bug then.<br>
Please file a bug report if you can confirm that behavior.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
6) In BinaryImagetoShapeLabelMap<br>
there is SetInputForegroundValue to modify the foreground but<br>
there is no SetInputBackgroundValue. Likewise, SetOutputBackgroundValue<br>
exists, but SetOutputForegroundValue does not exists.<br>
I would expect the missing functions to be available unless<br>
there is good reason to not to.<br>
</blockquote>
<br></div>
I would say there wouldn't be any good reason to add them, because they wouldn't be used.<br>
Can you provide a use case where they could be used? </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
7) The return type for RemoveLabel, RemoveLabelObject in<br>
LabelMap class are void. STL map tells how many<br>
objects are removed with the corresponding function prototype.<br>
This information can be provided to the caller.<br>
</blockquote>
<br></div>
That would be a nice feature.<br>
Could you provide a patch or file a bug report ?<br>
<br>
Regards,<br>
<br>
Gaëtan<br><font color="#888888">
<br>
-- <br>
Gaëtan Lehmann<br>
Biologie du Développement et de la Reproduction<br>
INRA de Jouy-en-Josas (France)<br>
tel: <a href="tel:%2B33%201%2034%2065%2029%2066" value="+33134652966" target="_blank">+33 1 34 65 29 66</a> fax: 01 34 65 29 09<br>
<a href="http://mima2.jouy.inra.fr" target="_blank">http://mima2.jouy.inra.fr</a> <a href="http://www.itk.org" target="_blank">http://www.itk.org</a><br>
<a href="http://www.bepo.fr" target="_blank">http://www.bepo.fr</a><br>
<br>
</font></blockquote></div><br>