Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3521 closed Bug (fixed)

rounding issue in tr_truncd()

Reported by: kovalev Owned by: charles
Priority: Low Milestone: 2.12
Component: libtransmission Version: 2.11
Severity: Trivial Keywords: backport-2.0x
Cc: iakovalev@…

Description

Affects seed ratio displayed as "Goal:" in torrent info field of Transmission GTK main window. Under some circumstances it's being displayed rounded-down.

To reproduce: select option "Torrent">"Properties">"Options">"Seed torrent until its ratio reached" and enter torrent-specific seed value directly into the field (say, 2.20). Look at "Goal:" value in torrent's info field of main window (must be 2.20). Now do the same, but use arrows at the right of the field instead of entering new value directly. There is a chance that "Goal:" will be displayed as 2.19, with "Seed torrent until..." is still being 2.20.

Versions affected: Transmission GTK since at least 1.83 to 2.04 release 11151.

Attachments (6)

Rounding.issue.png (96.2 KB) - added by kovalev 11 years ago.
remove-tr_truncd-nonmac-parts.diff (8.1 KB) - added by charles 11 years ago.
removeTruncd.patch (11.3 KB) - added by charles 11 years ago.
revised patch that (1) uses Longinus00's suggestion for the radix, (2) addresses the truncd(3.9999,1) issue, and (3) adds a possible mac implementation
removeTruncd.2.patch (11.5 KB) - added by charles 11 years ago.
revised patch which uses DBL_MANT_DIG as discussed in #transmission
removeTruncd.3.patch (11.7 KB) - added by Longinus00 11 years ago.
Switch to DBL_EPSILON to be more explicit.
epsilon.patch (2.1 KB) - added by charles 11 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 11 years ago by charles

Hey kovalev,

Would it be possible for you to attach a screenshot of this?

Changed 11 years ago by kovalev

comment:2 Changed 11 years ago by kovalev

Here it is - check numbers in "Torrent properties" - "Options" - "Seed torrent until..." and in main window.

To reproduce: select the option and try to modify seed ratio clicking on arrows several times.

comment:3 Changed 11 years ago by charles

Thanks!

comment:4 Changed 11 years ago by charles

  • Component changed from GTK+ Client to libtransmission
  • Keywords backport-2.0x added
  • Milestone changed from None Set to 2.10
  • Status changed from new to assigned
  • Summary changed from Minor rounding issue in GTK main window - seed ratio to rounding issue in tr_truncd()

comment:5 Changed 11 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in trunk by r11252

comment:6 Changed 11 years ago by kovalev

  • Cc iakovalev@… added
  • Milestone 2.10 deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 2.04 to 2.10

Have to reopen this ticket since the issue is still present in r11297 - though its manifestation has become more rare and far more reproducible.

To reproduce: try to increment/decrement seed ratio of any seeding torrent (with download finished) via "Torrent" - "Properties" - "Options" dialogue by 0.65 to 0.85 (actually, this might be dependent on the size of data). Use only arrows right to the data field. Watch "Goal:" value in main window as described above.

comment:7 Changed 11 years ago by charles

  • Milestone set to 2.11
  • Status changed from reopened to new

confirmed... ugh

comment:8 Changed 11 years ago by charles

  • Status changed from new to assigned

Fixed in trunk by r11298

comment:9 Changed 11 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:10 Changed 11 years ago by kovalev

  • Resolution fixed deleted
  • Status changed from closed to reopened

Well, in r11329 the issue has become even more reproducible than in r11297. Now at least it appears at some fixed values of seed ratio regardless of the size of data. To reproduce: set seed ratio to 1.80-1.95 then try to increment it to 2.05 and above as descried previously. Watch how "Goal:" display error appears at 2.05 and persists later on. The behaviour upon decrementing is somewhat different though - the display error seems does not accumulate.

Hope it will help to nail down this mysterious nuisance...

comment:11 Changed 11 years ago by charles

gaaaah!

comment:12 Changed 11 years ago by charles

kovalev: now, go and break r11331... ;)

comment:13 Changed 11 years ago by Longinus00

So my this is probably due to the fact that .05 (1/20) can't be represented as a terminating decimal in base2 (and thus IEEE 754). Instead of funking around with tr_truncd, which is mostly used for displaying progressbars, why not just modify tr_strtruncd which already has a printf?

diff --git libtransmission/utils.c libtransmission/utils.c
index 1c937ab..51d553e 100644
--- libtransmission/utils.c
+++ libtransmission/utils.c
@@ -1440,7 +1440,9 @@ tr_truncd( double x, int decimal_places )
 char*
 tr_strtruncd( char * buf, double x, int precision, size_t buflen )
 {
-    tr_snprintf( buf, buflen, "%.*f", precision, tr_truncd( x, precision ) );
+    assert( (int)log10(x) + precision + 2 < (int)buflen );
+    tr_snprintf( buf, buflen, "%.*f", precision + 1 , x );
+    *(strchr(buf,'.') + precision + 1) = '\0';
     return buf;
 }
 

The assert might be overkill...

comment:14 Changed 11 years ago by Longinus00

Looking at my code you could replace the "+ 1"s with a ++precision in the printf.

comment:15 Changed 11 years ago by Longinus00

The final option is to not format json real values with tr_truncd.

comment:16 Changed 11 years ago by charles

I like the idea of removing tr_truncd() and folding it all into tr_strtruncd() since that's the only real use.

Though there are these uses in the mac client. I'm not sure whether there are any subtle differences between *printf() and localizedStringWithFormat so let's get livings to weigh in.

macosx/NSStringAdditions.m:            return [NSString localizedStringWithFormat: @"%.2f", tr_truncd(ratio, 2)];
macosx/NSStringAdditions.m:            return [NSString localizedStringWithFormat: @"%.1f", tr_truncd(ratio, 1)];
macosx/NSStringAdditions.m:            return [NSString localizedStringWithFormat: @"%.0f", tr_truncd(ratio, 0)];
macosx/NSStringAdditions.m:        return [NSString localizedStringWithFormat: @"%.2f%%", tr_truncd(progress * 100.0, 2)];
macosx/NSStringAdditions.m:        return [NSString localizedStringWithFormat: @"%.1f%%", tr_truncd(progress * 100.0, 1)];

comment:17 Changed 11 years ago by livings124

I believe that works like printf. All that's needed is a simple way to get the truncated value.

comment:18 Changed 11 years ago by charles

livings124: could you change those mac client lines to use tr_strtruncd() instead of tr_truncd()? Then we can take tr_truncd() out of the public namespace altogether.

Changed 11 years ago by charles

comment:19 Changed 11 years ago by kovalev

The display issue seems has been fixed in r11336.

Last edited 11 years ago by kovalev (previous) (diff)

comment:20 Changed 11 years ago by charles

The change in r11331 is definitely broken, as is longinus00's patch in comment:13 ... both of us are assuming '.' is always going to be used for a decimal point.

Whether we get rid of tr_truncd() or not, if we're going to use this approach we'll need to either (a) temporarily set LC_NUMERIC to NULL before the call to printf() or (b) use nl_langinfo(RADIXCHAR).

Longinus00, do you have an opinion on this?

comment:21 Changed 11 years ago by Longinus00

How about this?

strchr(buf,localeconv()->decimal_point)[precision+1] = '\0';

I guess this might have issues if thousands_sep is the same as decimal_point (is that even legal?). So the LC_NUMERIC approach might work best.

Last edited 11 years ago by Longinus00 (previous) (diff)

comment:22 Changed 11 years ago by charles

Ah perfect, it's even part of c89. I'd forgotten about struct lconv.

comment:23 Changed 11 years ago by charles

In testing, this new revision still fails if we do something like trying to truncate 3.9999 to one decimal point -- printf("%.*f",precision+1,x) rounds it to 4.00.

We can ameliorate this by adding more decimal places to our call to snprintf()...

Changed 11 years ago by charles

revised patch that (1) uses Longinus00's suggestion for the radix, (2) addresses the truncd(3.9999,1) issue, and (3) adds a possible mac implementation

Changed 11 years ago by charles

revised patch which uses DBL_MANT_DIG as discussed in #transmission

Changed 11 years ago by Longinus00

Switch to DBL_EPSILON to be more explicit.

comment:24 Changed 11 years ago by charles

livings124, are the mac pieces of removeTruncd.3.patch okay?

comment:25 Changed 11 years ago by livings124

Nope, unfortunately it doesn't reflect the system's number format (decimal vs. comma, etc.).

comment:26 Changed 11 years ago by charles

Let's leave tr_truncd() in then.

Here's a patch that adds the new-and-improved version of tr_truncd() from removeTruncd3.patch, and also adds a case to handle strstr(buf,decimal_point) returning NULL in some circumstances such as NaN being passed in.

livings, could you test this patch out on the Mac client before I check it in and close this ticket?

Changed 11 years ago by charles

comment:27 Changed 11 years ago by livings124

That appears to work. My only suggestion is to get rid of the static variable - a decent compiler should be able to handle that.

comment:28 Changed 11 years ago by charles

Backgrounder on why the static variable isn't needed in gcc: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29335

comment:29 Changed 11 years ago by charles

  • Milestone changed from 2.11 to 2.12
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version changed from 2.10 to 2.11

Committed in r11345 w/o the static variable

comment:30 Changed 11 years ago by charles

note if/when backporting to 2.0x: after all these iterations, at least as of r11345 the only backporting needed would be its new implementation of tr_truncd().

comment:31 Changed 11 years ago by charles

backported to 2.0x by r11474

Note: See TracTickets for help on using tickets.