Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time unit fixes #386

Merged
merged 4 commits into from
Jul 22, 2021
Merged

Time unit fixes #386

merged 4 commits into from
Jul 22, 2021

Conversation

will-moore
Copy link
Member

Fixes #385

This PR handles time values up to Days (previously, anything over 99 hours was not displayed correctly).
It also handles any time units.

Tested using script at #385 (comment) to set units for existing image.

When doing this for a sample (FRAP) image, timestamps that are less than a day, no 'days' are shown:

Screenshot 2021-07-06 at 16 02 33

But when the time is longer than a day it is show like this (NB: open to suggestions for best way to format days)?

Screenshot 2021-07-06 at 16 01 58

If the time is an exact number of days, e.g. 3 days, we still show hh:mm:ss.sss. Is it worth eliminating that if all those values are zero?

cc @jburel

@jburel
Copy link
Member

jburel commented Jul 6, 2021

If the time is an exact number of days, e.g. 3 days, we still show hh:mm:ss.sss. Is it worth eliminating that if all those values are zero?

My vote is yes
Also the precision ss.sss when hours or even minutes are displayed seems too much
From your screenshot it looks like the time is a negative value when no days are displayed

@will-moore
Copy link
Member Author

Hmmm - I'm not sure I agree. When my digital clock says 11 o'clock, it's 11:00 not 11. Same for a stop-watch. The display doesn't change just because you hit a round number.

If move the T-slider from 1 day to 1 day 00:00:00.001 then there's going to be a big shift in the length of the displayed time string, causing the slider to contract to provide more space.

If a user sees time displayed as 1 day they won't know whether that's really "1 day 2 hours" or "1 day 1 second" rounded to 1 day, or if it's precisely 1 day. Showing the full accuracy and being consistent across all time-points is much clearer.

If we take a more advanced strategy later, and we display times differently depending on the units on the server, then we could consider some other behaviours (as well as showing the server-side units), but that is a whole different change.

The negative values in that screenshot are correct. It's a FRAP movie where frames before the bleach event have negative timestamps.

I ran the script #385 (comment) on this FRAP Image (user-3):
https://merge-ci.openmicroscopy.org/web/webclient/img_detail/83221/ and this DV: https://merge-ci.openmicroscopy.org/web/webclient/?show=image-122915

@will-moore
Copy link
Member Author

An example from IDR where this will affect the formatting: https://idr.openmicroscopy.org/webclient/img_detail/10340802/

96:00:00.000 will be 4 days 00:00:00.000.

@sbesson
Copy link
Member

sbesson commented Jul 12, 2021

Following up on the comment from #386 (comment) and this morning's discussion, my biggest concern is about the custom nature of the proposed format. For usability, I agree it makes complete sense to convert the information into the most relevant unit i.e. 4 days is clearer and more explicit than 96h. It also has the benefit of keeping the time suffix compatible with ISO 8601 where the hour is bounded between 00 and 23.

My main issue is that the proposed representation mixes unit written as words and unit separators which I found confusing. I would have almost expect something 4 days 4 hours 2 minutes 10 seconds. Otherwise, would a truncated version of the ISO standard i.e. dd hh:mm:ss[.mmm] be an option?

@jburel
Copy link
Member

jburel commented Jul 12, 2021

4 days 4 hours 2 minutes 10 seconds will be too long to fit in the available space.
So I think the suggested truncated version of the ISO standard is probably the most appropriate one

@will-moore
Copy link
Member Author

I've fixed the formatting, adding a tooltip to improve clarity:

Screenshot 2021-07-13 at 16 15 31

Needed to change a bit more code, so please test values shown while sliding, beside the slider AND tooltip.

@jburel
Copy link
Member

jburel commented Jul 15, 2021

Looking at https://merge-ci.openmicroscopy.org/web/webclient/img_detail/83221/
If possible we could be a be smarter in the tooltip 1 days always makes me jump

@will-moore
Copy link
Member Author

@jburel Done

@jburel
Copy link
Member

jburel commented Jul 21, 2021

I have installed this PR in pilot-0113
The time is now displayed in days instead of seconds.
Screenshot 2021-07-21 at 15 24 07

@jburel
Copy link
Member

jburel commented Jul 21, 2021

The tooltip still displays 0hours but this can be fixed in a follow-up PR

@jburel
Copy link
Member

jburel commented Jul 21, 2021

@jburel
Copy link
Member

jburel commented Jul 21, 2021

suggest to merge it and release as 0.11.1

@jburel
Copy link
Member

jburel commented Jul 22, 2021

Merging and tagging

@jburel jburel merged commit 0e9021d into ome:master Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Units in planeInfo
3 participants