Z-offset attribute label

[Rdornier]

Should fix the issue #513 for labels "Z:[z.unit; offset=2]". For z.pixel attribute, the offset is not taken into account in order to be coherent with the time stamps labels behavior.

[will-moore]

I tested a bit more to compare the behaviour with time labels offset... This is actually a lot more complex than I thought!

In the time offset case, you can choose a delta-T offset by using the T-index. For example (see screenshot) if you have an event at T:10 where the time-stamp is 18:53 (2nd panel) and you want to show time events relative to that time, then you can use offset=10, which is used for the Red and Blue labels. The Blue labels show time (min:sec) and the times are reduced by 18:53, which is the 9th deltaT value (since when we show T:10 that is actually theT=9 because we display theT + 1 (this is the same as all other viewers in OMERO, iviewer, insight etc, but it adds to the confusion).

If we apply the same logic to Z-index I think it should look like this: (Red and Blue labels are offset=10, Red are Z-index, Blue are Z.unit):

Does this look right to you? This subtracts the Z-offset but the code you've had so far adds the Z-offset.

I think this is consistent with T-index behaviour but the code to get this to behave like this is kinda ugly... NB: the Green labels above are offset=-10 and this doesn't work for time since we can't find the deltaT[-10] or any negative value, but it does work for Z-index.

diff --git a/src/js/models/panel_model.js b/src/js/models/panel_model.js
index 14bc6fa..ff7a897 100644
--- a/src/js/models/panel_model.js
+++ b/src/js/models/panel_model.js
@@ -297,8 +297,9 @@
             var shiftIdx;
             if (ref_idx) {
                 shiftIdx = parseInt(ref_idx)
-                var shift = this.get('deltaT')[shiftIdx];
-                deltaT = shift==null ? deltaT : shift;
+                // e.g. User enters "11", the T index is 10
+                var shift = this.get('deltaT')[shiftIdx - 1];
+                deltaT = shift==null ? deltaT : deltaT - shift;
             }
             var isNegative = (deltaT < 0);
             deltaT = Math.abs(deltaT);
@@ -307,7 +308,7 @@
             if (format === "index") {
                 isNegative = false;
                 if(!isNaN(shiftIdx) && shiftIdx > 0)
-                    text = "" + (theT + shiftIdx + 1);
+                    text = "" + (theT - shiftIdx + 1);
                 else text = "" + (theT + 1);
             } else if (['milliseconds', 'ms'].includes(format)) {
                 text = (deltaT*1000).toFixed(dec_prec) + " ms";
@@ -397,16 +398,19 @@
                     var theZ = this.get('theZ');
                     var deltaZ = theZ;

+                    var shift;
                     if (ref_idx) {
-                        var shift = parseInt(ref_idx)
-                        if(!isNaN(shift)){
-                            deltaZ = theZ + shift;
-                        }
+                        shift = parseInt(ref_idx)
                     }
                     if (format === "pixel") {
+                        if(!isNaN(shift)){
+                            deltaZ = theZ - (shift);
+                        }
                         text = "" + (deltaZ + 1);
-                        
                     } else if (format === "unit") {
+                        if(!isNaN(shift)){
+                            deltaZ = theZ - (shift - 1);
+                        }
                         text = ""+ (deltaZ * z_size).toFixed(dec_prec) +" "+ z_symbol
                     }
                 }

[Rdornier]

Thanks for your detailed explanation. Ok, I wanted to homogenize with time but I went wrong with the Z behavior from the beginning. I took the offset in the opposite direction. So, yes, you're right, we need to subtract the shift.

For the consistency, I have few more questions

1. The way it is now (with the corrections applied) allows negative offset only for Z, not for T, whatever is unit or index. Should we keep this difference or allows negative offset for T or restrict to positive offset only for Z ?
2. In the case where the offset is larger than the dataset size, deltaT[shiftIdx-1] doesn't exist anymore. It means the time[min:sec];offset=100 gives the current time position (Magenta) and not the offset position whereas the index is shifted (blue) (see figure below). For a Z stack, both index and unit position are shifted by the offset. Shall we by-pass deltaT[shiftIdx-1] and make it work for larger offsets or just restrict the index ?

I would go to modify the restriction forced by deltaT[shiftIdx-1] to be consistent with the Z offset behavior but I don't know if the time increment is stored somewhere and if it always consistent within a time-lapsed image.

[will-moore]

Time is a bit more tricky than Z because we don't know that the timestamps are evenly distributed. For example, if you have a series of frames at these timestamps:

T:1 , T:2    , T:3    , T:4...
0   , 1 hour, 2 hours, 3 hours, 3hrs:1min, 3hrs:2mins, 3hrs:4mins, 3hrs:7mins, 3hrs:10mins...etc, 

The event of interest starts at 3 hours, so the user will enter offset=4 since the T-index for that frame is T:4. We do var shift = this.get('deltaT')[shiftIdx - 1]; to get the deltaT at deltaT[3] which is 3 hours. Then we subtract 3 hours from all the timestamps, so we get:

T:4. ,  T:5,  T:6...
0:00, 0:01, 0:02, 0:04, 0:07, 

If we did the same as for Z, were we subtract offset - 1 from theT index before looking up the deltaT, we'd simply show the timestamp value from 3 frames before, which is not what we want (this is very different from subtracting 3 hours from every timestamp):

T:4. ,  T:5,  T:6
0:00, 1:00, 2:00

The trouble with using this technique offset=3 => "subtract 3 hours from every timestamp" is that it won't work for negative offsets, since we can't lookup a timestamp for theT = -2 since there is no frame at theT=-2.

[will-moore]

I also just remembered that whatever we do in the JavaScript app, we also have to replicate in the Figure_to_pdf.py script so we get a matching PDF and TIFF exported!

We don't want to change the T offset handling (since that would also be a breaking change) so let's just make sure that theZ offset is working in the same way in the figure export.

[will-moore]

The problem we have with existing T-offset behaviour is that offset=1 does nothing, since that refers to the first frame! We probably don't want to replicate that behaviour for Z, so best to stick with deltaZ = theZ - shift;.

[will-moore]

The Z-offset behaviour will likely need some example in the 'tips' dialog, which actually does a good job of explaining the T-offset behaviour:

[Rdornier]

Hi @will-moore,

I finally manage to find time to get back to this. Ok for time, I've removed everything concerning time changes from that PR. It should be fine now. I also modify the Figure-to-pdf and the tips acoordingly.

Edit: I created a new PR to fix the time index offset that didn't update because of the theT

[will-moore]

Hi, since we're going for different behaviour than for T offset, we need a different way of describing it in the info panel.

Something like:

Z index with units, offset by the Z pixel size x 3 Z: [z.unit; offset=3] Z: -3.00 µm
Z index (first plane is Z: 1), offset by 3 Z: [z.pixel; offset=3] Z: -2 (when showing first plane)

Does that make sense?

[Rdornier]

Hi @will-moore,
You're right, it should be -2 instead of -3. I've corrected it and also did the rephrasing

[pwalczysko]

Works as expected on merge-ci. The lines in the Tips examples are formatted ok, and, although they are showing theoretical values, one might argue that you can deduce by playing with the feature, whilst starting from the example the behaviour and adjust it for your purposes. The label is dynamic which is a great strenght. Thank you @Rdornier

LGTM

[pwalczysko]

Sorry, I can see some probs with Pdf export script @Rdornier - see https://github.com/ome/omero-figure/pull/521#pullrequestreview-1746222892 cc @will-moore

See figure https://merge-ci.openmicroscopy.org/web/figure/file/12325/ user-3. Panel edited is the one in the last row which is moved slightly off

[will-moore]

That error seems not related to this PR. The panel is missing pixel_size_z_symbol so that

z_symbol = panel.get('pixel_size_z_symbol')

gives None which fails when we try to concatenate:

label_value = (z_pos + " " + z_symbol)

That code was added in https://github.com/ome/omero-figure/pull/472/files, which also included code to populate that field when adding images to the figure, but this panel was probably added before that PR.

If you add that Image to a new figure, it won't give you this error.

I'll open a different PR to fix that.

[pwalczysko]

I'll open a different PR to fix that.

Indeed, #531 fixes the problem. I was able to export the figure tested in this PR without problems, both as pdf as well as tiff. Approving this PR again, sorry for the back and forth @Rdornier