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