[ImageJ-devel] proposed changes in the ImgLib2 abstract class hierarchy
Stephan Saalfeld
saalfeld at mpi-cbg.de
Wed Mar 21 03:48:22 CDT 2012
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