[ImageJ-devel] proposed changes in the ImgLib2 abstract class hierarchy
Stephan Saalfeld
saalfeld at mpi-cbg.de
Thu Mar 22 16:52:46 CDT 2012
Surprising! That means that the runtime check is somehow erased by the
compiler---cool. Probably a special optimization rule for the oh so
common 'constructors with a boolean'. Ok---if it is like that then I
have no concerns going for your suggestion.
Best,
Stephan
On Wed, 2012-03-21 at 13:25 -0500, Curtis Rueden wrote:
> 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
> >
> >
> >
> >
> >
>
>
>
More information about the ImageJ-devel
mailing list