[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