[ImageJ-devel] proposed changes in the ImgLib2 abstract class hierarchy

Tue Mar 20 23:17:59 CDT 2012

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<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<http://imagej.net/mailman/listinfo/imagej-devel>
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://imagej.net/pipermail/imagej-devel/attachments/20120320/da88f590/attachment-0001.html>