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

[#71] Fix rounding issues in format_time_minutes #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Giles314
Copy link

@Giles314 Giles314 commented Aug 5, 2024

The fix is double:

  • First integers are used for computation instead of Timestamp. This single part should be sufficient to suppress the observed anomaly. It will ensure that the days, hours, minutes, and seconds are correctly computed relying on mathematically perfect representation of integers with an integer type. Using integer we can be sure than we will avoid display of 60 seconds, 60 minutes or 24 hours.
  • But this may leave an anomaly when displaying factional part of the time representation. Assuming abs(t) Timestamp is a value which is very close to 2 (1.999...) so floor of this value should obtain 1.00.... As the result we print the following integer part: 00:01. When printing faction part, abs(t) - floor(abs(t)) the result will be 0.9999.... Then we display this factional value and Boost should produce 1.000 when requested a precision of 3, if it does correct closer value rounding. Then we will keep only 000 leading to a final result 00:01.000 far different from expected 00:02.000. So the second fix consists in checking the carry when rendering the fraction part and to add it to the integer part.

@mike42
Copy link

mike42 commented Aug 30, 2024

Just confirming that I'm also seeing the test failures which are fixed by this change.

Before:

$ /home/mike/workspace/sigrok/sigrok-util/cross-compile/linux/build/pulseview/build/test/pulseview-test 
Running 11 test cases...
/home/mike/workspace/sigrok/sigrok-util/cross-compile/linux/build/pulseview/test/util.cpp(220): error: in "UtilTest/format_time_minutes_test": check fmt(ts(12000), 0) == "+3:20:00" has failed [+3:19:60 != +3:20:00]
/home/mike/workspace/sigrok/sigrok-util/cross-compile/linux/build/pulseview/test/util.cpp(221): error: in "UtilTest/format_time_minutes_test": check fmt(ts(15000), 0) == "+4:10:00" has failed [+4:09:60 != +4:10:00]

*** 2 failures are detected in the test module "Master Test Suite"

But with the changes in this PR applied:

$ /home/mike/workspace/sigrok/sigrok-util/cross-compile/linux/build/pulseview/build/test/pulseview-test
Running 11 test cases...

*** No errors detected

I'm assuming these tests worked when they were written, so extra detail on my setup: I'm compiling from source on Debian trixie using the sigrok-cross-linux script in sigrok-util, with all dependencies pulled in via apt (list on the Wiki).

I've also included a short animation here showing that I've tested the change in the actual application. The commit ID is visible in the about screen, and I'm dragging a time range showing that the numbers are still formatting as the user would expect.

Kooha-2024-08-30-10-38-53

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.

2 participants