Z-offset attribute label #521

Rdornier · 2023-10-03T08:12:56Z

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.

src/js/models/panel_model.js

will-moore · 2023-10-18T08:55:28Z

src/js/models/panel_model.js

+                        if(!isNaN(shift)){
+                            deltaZ = theZ + shift;
+                        }
+                    }
                    if (format === "pixel") {
                        text = "" + (theZ + 1);


I wonder if you could also use deltaZ instead of theZ here, so that you can use offset when showing Z index.
And if you could apply the same change to theT that would be great.
There's maybe not much use case for that (I only noticed it in testing) but that's still the way you'd expect it to behave.
In my test, the white labels (offset=2) show the same as yellow labels (no offset):

Ok make sense.
Is it what you had in mind ?

Sorry, no! I just meant for that line to be text = "" + (deltaZ + 1); instead of text = "" + (theZ + 1);
That's all! Apologies for the confusion!

Ah...Ok, now I see. No worries, I didn't correctly read.
I've corrected it. I also correct a small bug on time offset (see below) but I noticed that the current implementation doesn't allow for negative offsets for time (but allowed for Z)

var shift = this.get('deltaT')[shiftIdx]; deltaT = shift==null ? deltaT : shift;

Do we also allow it for time ?

…offset and fix bug on time offset

will-moore · 2023-10-19T22:30:45Z

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 · 2023-10-20T08:12:40Z

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

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 ?
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 · 2023-10-20T09:29:14Z

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 · 2023-10-20T09:31:36Z

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 · 2023-10-24T11:35:18Z

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 · 2023-10-24T11:43:39Z

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 · 2023-10-27T13:28:07Z

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 · 2023-11-14T11:57:56Z

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 · 2023-11-14T15:07:31Z

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

will-moore · 2023-11-20T13:04:45Z

omero_figure/templates/figure/index.html

                            <td>Z: [z.unit; offset=3]</td>
                            <td>Z: -3.00 &#181;m </td>
                        </tr>
                        <tr>
-                            <th>Z slice with index position <br/>relative to the 3rd frame</th>
+                            <th>Z index with (first plane is Z: 1) <br/>offset by 3</th>


I think we don't need the with in this case? Otherwise looks good.

True, thanks. !
This is a typo.

will-moore

Thanks 👍

pwalczysko · 2023-11-23T11:05:26Z

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

Traceback (most recent call last):
  File "./script", line 2537, in <module>
    run_script()
  File "./script", line 2523, in run_script
    file_annotation = export_figure(conn, script_params)
  File "./script", line 2479, in export_figure
    return fig_export.build_figure()
  File "./script", line 978, in build_figure
    self.add_panels_to_page(panels_json, image_ids, page)
  File "./script", line 1977, in add_panels_to_page
    self.draw_labels(panel, page)
  File "./script", line 1277, in draw_labels
    label_value = (z_pos + " " + z_symbol)
TypeError: can only concatenate str (not "NoneType") to str

When executing the Figure_to_Pdf.py script.

pwalczysko · 2023-11-23T11:13:26Z

Sorry, I can see some probs with Pdf export script @Rdornier - see #521 (review) 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 · 2023-11-23T11:29:07Z

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 · 2023-11-23T13:38:52Z

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

… vite

Rdornier added 4 commits October 3, 2023 09:26

Add z-offset option in labels

dcecb76

Integrate the z-offset in the existing functions and fix bugs

18e9ee5

extend offset beyond z-limits

b3061c7

Clean code

64c7983

will-moore reviewed Oct 5, 2023

View reviewed changes

src/js/models/panel_model.js Outdated Show resolved Hide resolved

will-moore added the include label Oct 5, 2023

fix case where offset is NaN

c48a1ef

will-moore reviewed Oct 18, 2023

View reviewed changes

Rdornier added 4 commits October 18, 2023 17:19

Add z-offset value to the label

15df5dc

Refactor

d8e7601

Add Time offset to label

7212b38

Remove addings in label; modified the index label accordingly to the …

6bb1381

…offset and fix bug on time offset

Correct bug on positive offset

2190724

Rdornier added 5 commits October 27, 2023 14:38

remove offset +1

90522d7

Update figure to pdf script

ffd83e9

Fix offsets

8a28043

Add Z tips

035da8f

coming back to the first behavior for time offset

b548f82

Rdornier mentioned this pull request Oct 27, 2023

Enable time index offset #524

Merged

fix description in info panel

a35a596

will-moore reviewed Nov 20, 2023

View reviewed changes

Fix typo

68f10d8

will-moore approved these changes Nov 22, 2023

View reviewed changes

will-moore added this to the 6.1.0 milestone Nov 22, 2023

Merge branch 'ome:master' into z-offset

f0904f3

pwalczysko approved these changes Nov 23, 2023

View reviewed changes

pwalczysko requested changes Nov 23, 2023

View reviewed changes

will-moore mentioned this pull request Nov 23, 2023

Fix missing pixel_size_z_symbol in Figure_To_Pdf.py #531

Merged

pwalczysko self-requested a review November 23, 2023 13:39

pwalczysko approved these changes Nov 23, 2023

View reviewed changes

will-moore merged commit e310ddb into ome:master Nov 30, 2023
1 check passed

will-moore added a commit to will-moore/figure that referenced this pull request Feb 15, 2024

Merge commit '68f10d8a' from PR ome#521 Z-offset attribute label into…

5068c7e

… vite

Rdornier deleted the z-offset branch June 13, 2024 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Z-offset attribute label #521

Z-offset attribute label #521

Rdornier commented Oct 3, 2023

will-moore Oct 18, 2023 •

edited

Loading

Rdornier Oct 18, 2023

will-moore Oct 18, 2023

Rdornier Oct 19, 2023 •

edited

Loading

will-moore commented Oct 19, 2023 •

edited

Loading

Rdornier commented Oct 20, 2023

will-moore commented Oct 20, 2023

will-moore commented Oct 20, 2023

will-moore commented Oct 24, 2023

will-moore commented Oct 24, 2023

Rdornier commented Oct 27, 2023 •

edited

Loading

will-moore commented Nov 14, 2023

Rdornier commented Nov 14, 2023

will-moore Nov 20, 2023

Rdornier Nov 21, 2023

will-moore left a comment

pwalczysko commented Nov 23, 2023

pwalczysko left a comment

pwalczysko commented Nov 23, 2023 •

edited

Loading

will-moore commented Nov 23, 2023

pwalczysko commented Nov 23, 2023

Z-offset attribute label #521

Z-offset attribute label #521

Conversation

Rdornier commented Oct 3, 2023

will-moore Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Rdornier Oct 18, 2023

Choose a reason for hiding this comment

will-moore Oct 18, 2023

Choose a reason for hiding this comment

Rdornier Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

will-moore commented Oct 19, 2023 • edited Loading

Rdornier commented Oct 20, 2023

will-moore commented Oct 20, 2023

will-moore commented Oct 20, 2023

will-moore commented Oct 24, 2023

will-moore commented Oct 24, 2023

Rdornier commented Oct 27, 2023 • edited Loading

will-moore commented Nov 14, 2023

Rdornier commented Nov 14, 2023

will-moore Nov 20, 2023

Choose a reason for hiding this comment

Rdornier Nov 21, 2023

Choose a reason for hiding this comment

will-moore left a comment

Choose a reason for hiding this comment

pwalczysko commented Nov 23, 2023

pwalczysko left a comment

Choose a reason for hiding this comment

pwalczysko commented Nov 23, 2023 • edited Loading

will-moore commented Nov 23, 2023

pwalczysko commented Nov 23, 2023

will-moore Oct 18, 2023 •

edited

Loading

Rdornier Oct 19, 2023 •

edited

Loading

will-moore commented Oct 19, 2023 •

edited

Loading

Rdornier commented Oct 27, 2023 •

edited

Loading

pwalczysko commented Nov 23, 2023 •

edited

Loading