[ImageJ-devel] [ImgLib2] PLEASE READ IF YOU ARE USING VIEWS - Change of Views.translate() sign

[Tobias Pietzsch]

Hi all,

I just made a change to the Views.translate() function. It does now 
exactly the opposite of what it was doing before...

This will break your code if you use Views.translate(), and I apologize 
very much for the inconvenience. An explanation of why this was 
necessary is below, but if you just want to fix your code quickly:

   Simply replace all Views.translate() occurrences in your code by
   Views.offset().

Ok, so why was this done:
Until now, the translation vector t in Views.translate( source, tacted 
on the target coordinates, not the source coordinates. This is
counter-intuitive and inconsistent with the rest of Views. In
particular, if you have a RealView.affine( source, T ) with the affine
transform [0 t] this would have the behaviour of Views.translate(
source, -t).

This was pointed out by Albert Cardona a long time ago (see emails 
quoted below) and I've been putting it off ever since.  Now I wanted
to fix it before the ImageJ Conference, so that we can present a sane
Views.translate() version it the ImgLib tutorials there...

Again, sorry for the inconvenience.
best regards,
Tobias



On 11/28/2011 11:41 AM, Tobias Pietzsch wrote:
> Hi,
>
> As Albert points out, Views.translate( source, offset ) considers
> offset differently than IntervalIndexer.indexToPositionWithOffset(),
> etc.
> I agree that the current Views behaviour is counter-intuitive and
> should be changed.
>
> Right now,
>    targetView = Views.translate(sourceView, offset)
> gives you a view which starts at offset in sourceView. That is, the
> pixel at offset in sourceView corresponds to the origin of targetView.
> SourceView and targetView coordinates are related as
>    x_src = x_tgt + offset.
>
> We should rather have
>    targetView = Views.translate(sourceView, translation)
> to give
>    x_tgt = x_src + translation.
>
> I would not change anython in MixedTransformView because this does
> what you would expect it to do: It is constructed as
> MixedTransformView( source, transformToSource ), that is, the transform
> is given fomr target to source, as
>    x_src = transformToSource( x_tgt )
> For the current Views.translate this resulted in
>    transformToSource = offset,
> not requiring inversion of offset, which is probably, why I did it that
> way.  However, I agree it should be changed.
>
> For convenience, I would make another method which maintains the current
> behaviour, because it implements something which you often want.
> For instance, to "crop" an interval (min,max) from a view, you
> can currently do Views.translate(view, min). With the changed
> translate() you would have to invert min to get the translation,
> which would be inverted back inside translate() to get the
> transformToSource.
> I would call the new method something like shift() or offset().
>
> What do you think?
>
> best regards,
> Tobias
>
> On 11/25/2011 04:20 PM, Albert Cardona wrote:
>>
>> Hi Stephan, Tobias:
>>
>> the Views.translate considers a positive offset as negative.
>>
>> Is this perhaps a consequence of the change in how offsets are handled
>> now, meaning that MixedTransform needs a sign reversal somewhere?
>>
>> Albert
>>
>>
>>
>>
>> On Fri, 2011-11-04 at 10:39 +0100, Stephan Saalfeld wrote:
>>> Hi ImgLib2ers,
>>>
>>> in the last master commit Albert has found and fixed the first half of
>>> the long standing quirk in IntervalIndexer at calculating indices and
>>> positions with offsets (identified and reported a few weeks ago by
>>> Christion and Martin).  IntervalIndexer was consistently(!) treating
>>> offsets as negative offsets which was correct but unhandy using the
>>> positive min property in Intervals.  It is clear that inverting the
>>> logic in IntervalIndexer is a good idea.   To (hopefully) complete the
>>> process and get consistency back, I pushed the second half of the
>>> inversion now.  Either of both commits will break the KNIME code!  May
>>> be it will break other code too that is using IntervalIndexer.  I beg
>>> your pardon for being short in time and therefore not able to do
>>> extensive testings.  Please check your code and sorry for the
>>> inconvenience.  It's a short inconvenience for a better solution.
>>>
>>> Best regards,
>>> Stephan
>>>
>>>
>>>
>>>
>>>
>>> MHTML Document attachment, "Forwarded message - bug in
>>> IntervalIndexer.indexToPositionWithOffset with solution"
>>>> -------- Forwarded Message --------
>>>> From: Albert Cardona<acardona at ini.phys.ethz.ch>
>>>> To: Stephan Saalfeld<saalfeld at mpi-cbg.de>, Tobias Pietzsch
>>>> <tobias.pietzsch at inf.tu-dresden.de>, Stephan Preibisch
>>>> <preibisch at mpi-cbg.de>
>>>> Subject: bug in IntervalIndexer.indexToPositionWithOffset with
>>>> solution
>>>> Date: Thu, 03 Nov 2011 09:56:13 -0400
>>>>
>>>>
>>>> The 3 methods IntervalIndexer.indexToPositionWithOffset are subtracting
>>>> the offsets when they should be adding them.
>>>>
>>>> This affects the LocalizingIntervalIterator.jumpFwd, which is then
>>>> screwed up for the subclass RandomAccessibleIntervalCursor.
>>>>
>>>> Mind if I change them? These methods are not used anywhere else.
>>>>
>>>> Albert
>>>>
>>
>