Time reference and decimal places

[Tom-TBT]

Hello again, with this PR I want to solve two issues that can be handled in the dynamic labels:

• Specifying a frame index to serve as the "zero" timepoint https://github.com/ome/omero-figure/issues/442
• Specifying the decimal precision for time, that I then also found relevant for x, y, z, width and height https://github.com/ome/omero-figure/issues/441

A picture is worth a thousand words:

The decimal precision and offset (the "reference frame") are passed as named parameters inside the label:

• [time.seconds; precision=2] -> parameters are specified after a ;, in the format param_name=value
• [time.seconds; offset=3; precision=2] -> the order of the parameter is not important
• [time.seconds ; offset = 3 ;precision= 2 ] -> Spaces are not affecting the parsing
• [x.unit; offset=3; precision=2] -> Here the offset is not relevant for x.unit: it is ignored. The same happens with unknown parameters or invalid formats (no parameter name, no value, no = sign).

Finally, I added tests for the labels like @jburel suggested last time. I know that they should be run on github to test that nothing is crashing during the PDF conversion, but how would you run them locally to check visually that the output is the one we expect?

Cheers, Tom

#########################################

EDIT: The following is kept for reference about the discussion in this PR, but the format that was ultimately picked is described above.

UI-wise, here is what I chose:

• I specify the decimal precision as an integer instead of a series of '0'. The serie of '0' raises question about the implementation (should I count the characters, the zeros, what should it do when there is a mix). Also it would mean that the decimal precision can be 0 only when nothing is specified, but we already had 2 as the standard for x/y/z/width/height

• decimal precision comes after the format specification (secs, ms, mins, ...). Luckily the default for time/x/y.... are indexes, so having decimal precision for them doesn't make sense. Decimal precision is always in 3rd position, never 2nd:

  • [time.5] -> decimal precision of 5 for the index. No one needs that and the behavior is that the format isn't recognized
  • [time.index.5] -> this also doesn't make sense but still displays the index correctly (ignoring decimal places)
  • [time.seconds.2] -> Time in seconds with decimal precision of 2.
  • [x.unit.1] -> Position in X units (like µm) with decimal precision of 1.

• The frame reference for the time is given at the end, after a -. The time of that frame is subtracted from the other timepoints. Being the last parameter, it works both with and without decimal precision:

  • [t.secs.2-3] -> Decimal precision of 2 with reference frame n°3
  • [t.secs-3] -> Reference frame n°3
  • [t.secs-0] -> 0 is too low, reference frame not understood, ignoring the reference
  • [t-3] -> Here I decided not to implement it so it just shows the index ignoring the reference
  • [x.unit-3] -> Substracting 3rd pixel x-position? I also decided not to implement it. I don't see a use for it

[will-moore]

Thanks Tom, this looks really nice. Initial testing looks good - I need to do a bit more... It would be nice to have an example in the table that uses both reference and decimal places - otherwise it's not clear that you can combine them or how. E.g. t.s.-3.2 or t.s.2-3 ?

[will-moore]

The failed run X status of this PR seems to be because the test export script is failing at scale = panel['pixel_size_x'] (KeyError) probably because the image has no pixel size metadata.

Actually, I can reproduce the error by adding a label X: [x.unit] to an Image panel which has no pixel size (e.g. a png). So it makes sense to do scale = panel.get('pixel_size_x') and then handle scale is None.

Otherwise, testing all looks good 👍

[Tom-TBT]

The script failed there yes, but I wouldn't do the change in Figure_To_Pdf.py. I think that the test is wrong. The JSON looks like an old version! (pixel_size_x seems introduced in V1, looking at version_transform() in figure_model.js)

With the version transform, all pdf generated from now on should have the new JSON, right? (except if someone forgot to refresh his browser for quite some time now)

So I updated the JSON in the test (and reorganized it to make it look like the model in figure_model.js). If I'm right, there are also values added from panel_model (rotation, zoom, ...)

Let me know what you think about my JSON update, then let's see if the test passes this time 🤞

PS: Working on it, I spotted another bug in Figure_To_Pdf: label with [z.unit] on 2D images made it crash (z_symbol is None) I saved myself the effort of making another PR and fix the bug here. Is that ok for you?

[Tom-TBT]

Check failed again, and this time it comes from my JSON generation. I tried to get pixel_size_x similar to what is done in views.img_data_json, but while the imaging loading in OMERO.figure doesn't crash when the pixel size is NOT SET, here it complains because px is None (or similar).

When pixel size is NOT SET, the panel's JSON (talking about a normal figure) doesn't contain pixel_size_x -> testing on it the [x.unit] label, it displays NaN µm and the Pdf export crashes!

I think that I should handle it the same way as for Z and T, displaying 0 by default (and without adding pixel_size_x to the JSON, so that we keep the warning in the UI )

Tests are useful :D

[will-moore]

Testing the figure to Pdf with missing pixel_size_x is fixed now 👍 . Just a couple of wording suggestions above.

[will-moore]

After discussing @joshmoore and others this morning, we felt that something like this (which uses css-style formatting) would be easier to remember, more human readable and extensible: [time.unit; offset:3; precision:2]. That was my web-based preference, but Josh also suggested [time.unit; offset=3; precision=2] or you could go for JSON style [time.unit, offset:3, precision:2].

[joshmoore]

you could go for JSON style [time.unit, offset:3, precision:2].

Only slightly marred by there being no quotes :)

[Tom-TBT]

I will be on holiday for the next two weeks. Maybe best not to apply this change right now, so we have time to think about it.

But I like those "self-documenting formats" thanks a lot for the ideas!

[Tom-TBT]

Hello Moore², I ended up picking this style: [time.unit; offset=3; precision=2]. The : sign is already used for the h:m:s format so I wanted to avoid the confusion (though it's not a problem for the parsing, as ; is parsed first).

I also updated the tests, the tip window, and the figure_file_format documentation accordingly. Let me know!

[will-moore]

Sorry, not looked yet - I'm away till Wednesday... Please feel free to ping me again after then.

[will-moore]

Ah - I blindly tried [time.unit; offset=3; precision=2] for a time label (from the comment above) and this didn't get recognised... I realised after a while that time.unit is not valid.

Could you update the PR description with the new format (this can also serve as a reference)

[will-moore]

Functionality testing looks good. Labels are rendered the same in JavaScript and Python. Just a couple of minor comments above.

[will-moore]

Thanks Tom - some others on the team are away just now, but I when they're back we'll discuss if there's any outstanding issues/feedback. Cheers

[pwalczysko]

@Tom-TBT Sorry if I missed it in your previous PR:

Why is there the bounding box around the label such as Width: [width.pixel] please ? See screenshot below, FF, MacOS Big Sur. Is that expected ? Did I miss a hint about the formatting of such label ? It does not look natural or desirable on the figure...

Edit: sorry, I realized that this is because I copied and pasted into the label box couple of leading spaces. This is the cause of the strange formatting of the label later - still, pasting some spaces possibly should not have such consequences ? Will try other label types as well, possibly this is not something introduced in your PR, but just a consequence of the necessity to create the label formula by yourself.

Edit 2: Indeed, this happens also with labels such as Image Name, so most probably not caused by this PR. It is just that this PR exposes such flaws as it imposes a different workflow.

[pwalczysko]

Other than https://github.com/ome/omero-figure/pull/481#issuecomment-1287137105, this PR LGTM

[Tom-TBT]

Hi Petr, it looks to me that markdown is responsible for it.

Looking a bit around I found this: https://gist.github.com/dupuy/1855764#block-quotations quote: "Markdown treats four leading spaces as a code example"

So I don't know if bug or feature since we can now add code to the labels!!! ... Well maybe more like a bug, that's a weird way of attaching code to images

We could remove the leading spaces, but that could destroy someone's label alignment workflow. Let me see if I find a way of keeping the spaces without doing some markdown with it. Tell me if you would have that in this PR or in a separate one.

[Tom-TBT]

Well maybe that becomes hard to handle, I went through that tutorial https://www.markdownguide.org/basic-syntax/ and many works:

[pwalczysko]

thank you @Tom-TBT , this https://github.com/ome/omero-figure/pull/481#issuecomment-1288570316 explains a lot. I would say this all is for another PR, cc @will-moore might even have an issue/plan for this.

[will-moore]

Created an issue for the formatting behaviour the @pwalczysko reported: https://github.com/ome/omero-figure/issues/486 @jburel you happy to merge this?

[Tom-TBT]

Thank you Jean-Marie for the review, I made the changes

[jburel]

@Tom-TBT sorry for the delay. Thanks for making the adjustments

[will-moore]

Thanks @Tom-TBT - this is now released in omero-figure 5.1.0