Hi Stephan,<br><br><br><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
I agree that your version is easier to understand and cleaner to read<br>
but it introduces a runtime check that although being tiny, will slow<br>
down the wrapping of Points and RealPoints slightly.<br></blockquote><br>Thanks for the explanation. But I don't I buy it. Here is a little test:<br><br> <a href="https://gist.github.com/2150639">https://gist.github.com/2150639</a><br>
<br>On my system:<br><br>Five trials of RealPointSS yields: 418, 361, 357, 359, 370<br>Five trials of RealPointCR yields: 365, 357, 357, 372, 360<br><br>With each trial consisting of 10 million object creations.<br>
<br>The difference looks negligible to me.<br><br>Regards,<br>Curtis<br><br><br><div class="gmail_quote">On Wed, Mar 21, 2012 at 3:48 AM, Stephan Saalfeld <span dir="ltr"><<a href="mailto:saalfeld@mpi-cbg.de">saalfeld@mpi-cbg.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Curtis,<br>
<br>
since I am responsible for that construct, I take the freedom to reply.<br>
<br>
I agree that your version is easier to understand and cleaner to read<br>
but it introduces a runtime check that although being tiny, will slow<br>
down the wrapping of Points and RealPoints slightly. In situations<br>
where this is happening millions of times, I do not want the additional<br>
cost. So, I prefer your solution in business code but ImgLib2 aims to<br>
be performance code :). For clarity, the constructor is protected and<br>
therefore not exposed to the normal API user, a clear Javadoc should<br>
tell extenders about its expected use. Normally, you will use the<br>
RealPoint.wrap(double[]) factory method instead. We could make the<br>
constructor private but then we would lose the option to use it from the<br>
now extending classes.<br>
<br>
If this is still too ugly for your gusto, we could make the current<br>
RealPoint and Point AbstractRealPoint and AbstractPoint and introduce<br>
both this constructor and the static wrap method in the extending<br>
RealPoint and Point classes. I'd actually like that.<br>
<br>
Best,<br>
Stephan<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<br>
On Tue, 2012-03-20 at 23:17 -0500, Curtis Rueden wrote:<br>
> Hi Tobias,<br>
><br>
> First of all, thanks very much for all the API improvements!<br>
><br>
> I just have one comment...<br>
><br>
> Why is there the Object x in there?<br>
><br>
> If it wouldn't be there, then the compiler would be unable to<br>
> distinguish whether you call<br>
> protected Realpoint( final double[] position )<br>
> or<br>
> public RealPoint( final double... position )<br>
><br>
> Sure, the dummy Object parameter differentiates the constructors.<br>
> However, I do not believe such drastic measures are necessary. Why not<br>
> do something like this instead:<br>
><br>
> <a href="https://gist.github.com/2144264" target="_blank">https://gist.github.com/2144264</a><br>
><br>
> I think it is easier to understand and accomplishes the same ends.<br>
><br>
> Regards,<br>
> Curtis<br>
><br>
><br>
> On Fri, Mar 16, 2012 at 10:18 AM, Tobias Pietzsch<br>
> <<a href="mailto:pietzsch@mpi-cbg.de">pietzsch@mpi-cbg.de</a>> wrote:<br>
> Hi Steffi,<br>
><br>
> thanks a lot for having a look!<br>
><br>
><br>
> On 03/16/2012 03:33 PM, Stephan Preibisch wrote:<br>
> I have one question about a protected constructor in<br>
> RealPoint though:<br>
><br>
> /**<br>
> * Protected constructor that re-uses the passed<br>
> position array.<br>
> *<br>
> * @param position<br>
> * array used to store the position.<br>
> * @paramx<br>
> * unused parameter that changes the method signature<br>
> */<br>
> protected RealPoint( final double[] position, final<br>
> Object x )<br>
> {<br>
> super( position );<br>
> }<br>
><br>
> Why is there the Object x in there?<br>
><br>
><br>
> If it wouldn't be there, then the compiler would be unable to<br>
> distinguish whether you call<br>
> protected Realpoint( final double[] position )<br>
> or<br>
> public RealPoint( final double... position )<br>
><br>
><br>
> Also I have a small question about the CellImg. The<br>
> ListImgCells which<br>
> holds all cells is typed to DefaultCell, and also<br>
> always instantiates<br>
> DefaultCells. Shouldn't that be the part where we are<br>
> able to exchange<br>
> them for other types of cells? So shouldn't it be<br>
> typed to something<br>
> like C extends AbstractCell<A> or so? Or is there<br>
> another level where<br>
> you want to introduce that?<br>
><br>
><br>
> What we will exchange for the is the Cells< A, C > cells that<br>
> is passed to the CellImg constructor. ListImgCells implements<br>
> that interface such<br>
> that every Cell is stored as an Object. If we would alter the<br>
> Cell type<br>
> here, than there would still be an Object required for cells<br>
> that are<br>
> currently not in memory. So the idea was to change the Cells<<br>
> A, C ><br>
> implementation from ListImgCells to something else, that does<br>
> the caching and creates Cell objects when required.<br>
><br>
> Currently, I'm using "new DefaultCell< A >(...)" to create<br>
> cells to<br>
> create all the cells in the ListImgCells constructor. When we<br>
> type<br>
> ListImgCells to C extends AbstractCell<A>, we would need to<br>
> pass a<br>
> type variable into the constructor and add a createNewCell()<br>
> method<br>
> to the AbstractCell. This could be done, but let's wait until<br>
> we<br>
> actually need it.<br>
><br>
> The current idea is to exchange the Cells< A, C ><br>
> implementation.<br>
><br>
> best regards,<br>
> Tobias<br>
><br>
><br>
><br>
> Ciao ciao,<br>
> Steffi<br>
><br>
> On Mar 15, 2012, at 16:57 , Tobias Pietzsch wrote:<br>
><br>
> Hi,<br>
><br>
> I've implemented more changes to the ImgLib2<br>
> abstract class hierarchy.<br>
> This time, I restructured the integer<br>
> Positionables, Localizables, and<br>
> RandomAccesses.<br>
><br>
> While I was at it, I merged a new version of<br>
> CellImg (this is in<br>
> preparation for CellImgs that swap Cells to<br>
> disk) because I didn't<br>
> want to need to fix that for the new abstract<br>
> hierarchy later.<br>
> ListImg has also been changed a bit. And I<br>
> made some cosmetic changes<br>
> here and there...<br>
><br>
> I think I broke nothing and I would merge this<br>
> into master soon.<br>
><br>
> It would be totally awesome, if you could try<br>
> your ImgLib2 stuff with<br>
> the branch "modified-abstract-hierarchy" and<br>
> see if everything works<br>
> okay. If you have time to comment on the<br>
> changes I made - even better.<br>
><br>
> best regards,<br>
> Tobias<br>
><br>
> BTW: did you see the cool fractal example on<br>
> <a href="http://fiji.sc/wiki/index.php/ImgLib2_Documentation#A_RealRandomAccess_to_Render_Mandelbrot_Fractals" target="_blank">http://fiji.sc/wiki/index.php/ImgLib2_Documentation#A_RealRandomAccess_to_Render_Mandelbrot_Fractals</a><br>
> ?<br>
> :-)<br>
><br>
> On 03/13/2012 09:01 PM, Tobias Pietzsch wrote:<br>
> Hi guys,<br>
><br>
> while I was writing ImgLib docs, I<br>
> noticed that the current<br>
> abstract class hierarchy which leads<br>
> to AbstractRandomAccess<T><br>
> is a little bit stupid (see attached<br>
> imglib2-abstract-current.png)<br>
><br>
> There is no need for the abstract<br>
> class hierarchy to pull the<br>
> Sampler<T> interface all the way<br>
> through. The Sampler<T> is only<br>
> implemented in the concrete classes.<br>
> There is really no need to have<br>
> the dependency on T in the abstract<br>
> hierarchy. This prevents for<br>
> instance RealPoint to make use of the<br>
> RealPositionable and<br>
> RealLocalizable implementations in<br>
> AbstractRealRandomAccess<T>.<br>
><br>
> Therefore I propose to change the<br>
> hierarchy as shown in the<br>
> second attached diagram,<br>
> imglib2-abstract-proposed.png.<br>
><br>
> I implemented those changes for the<br>
> "real" path, see branch<br>
> "modified-abstract-hierarchy". Note<br>
> how nicely RealPoint fits<br>
> into the picture now :-)<br>
> Before I go ahead and do the same for<br>
> the integer accessors, I wanted<br>
> to ask whether you agree that this is<br>
> a good thing to do...<br>
><br>
> There is one more thing: The<br>
> AbstractRealRandomAccess implemented<br>
> the complete RealPositionable<br>
> interface, while AbstractRandomAccess<br>
> only implements part of Positionable -<br>
> with the implemented part<br>
> based on the unimplemented methods.<br>
> The idea was that if a derived<br>
> concrete accessor compiles you know<br>
> you implemented what was necessary.<br>
><br>
> However, I found the approach of<br>
> AbstractRealRandomAccess to implement<br>
> anything quite nice when using it. I<br>
> would suggest doing the same for<br>
> integer. Then either<br>
> * you know what you are doing when<br>
> overriding (parts of) the<br>
> Positionable implementation<br>
> * you leave it as is (e.g. Point)<br>
> * you derive from AbstractLocalizable<br>
> and implement Positionable<br>
> completely on your own.<br>
><br>
> Thoughts?<br>
><br>
> best regards,<br>
> Tobias<br>
><br>
><br>
><br>
> _______________________________________________<br>
> ImageJ-devel mailing list<br>
> <a href="mailto:ImageJ-devel@imagej.net">ImageJ-devel@imagej.net</a><br>
> <a href="http://imagej.net/mailman/listinfo/imagej-devel" target="_blank">http://imagej.net/mailman/listinfo/imagej-devel</a><br>
><br>
><br>
><br>
><br>
><br>
<br>
</div></div></blockquote></div><br>