Bug 1761 - Rounding with olsr::EmfToSeconds
Rounding with olsr::EmfToSeconds
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: olsr
ns-3-dev
PC All
: P5 normal
Assigned To: Gustavo J. A. M. Carneiro
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-11 11:34 EDT by Brian Swenson
Modified: 2015-11-01 05:56 EST (History)
4 users (show)

See Also:


Attachments
Simple sample program demonstrating the issue (324 bytes, text/x-c++src)
2013-09-11 11:34 EDT, Brian Swenson
Details
bug fix (614 bytes, patch)
2015-10-31 17:15 EDT, Tommaso Pecorella
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Swenson 2013-09-11 11:34:29 EDT
Created attachment 1669 [details]
Simple sample program demonstrating the issue

Hello

The following came up when I was comparing pcaps obtained running the regression tests for OLSR using the --int64x64-as-double configuration option in ns-3.

The pcaps with this option differ from the pcaps without this option.  One of the differences is with the validity time of messages.  These are converted using the functions EmfToSeconds and SecondsToEmf.  EmfToSeconds is rounding up even on small decimal values.

std::cout << EmfToSeconds(SecondsToEmf(15.0)) << std::endl;
std::cout << EmfToSeconds(SecondsToEmf(15.000000000000002)) << std::endl;
std::cout << EmfToSeconds(SecondsToEmf(15.1)) << std::endl;

returns the following:

15
15.5
15.5

EmfToSeconds and SecondsToEmf are located in src/olsr/model/olsr-header.cc/.h

When using --int64x64-as-double there ends up being a small decimal value on the original time which appears to be automatically rounded up.  Thus the pcaps are different and the regression tests fail.  Since it appears that SecondsToEmf is only accurate to .5 second perhaps the function should first round the value to be converted?
Comment 1 Peter Barnes 2014-03-02 04:14:46 EST
Brian, could you please check this again with

Bug 1856 Patch r10637 67601c471c22
http://code.nsnam.org/ns-3-dev/rev/67601c471c22
Comment 2 Tommaso Pecorella 2015-10-31 17:15:12 EDT
Created attachment 2170 [details]
bug fix

Dunno if the problem with int128 is still there, but the problem isn't the int128.

The issue is that the function is rounding numbers to the highest fraction, and not to the nearest. The standard is quite explicit on this:
     -    compute the expression 16*(T/(C*(2^b))-1), which may not be a
          integer, and round it up.  This results in the value for 'a'

Notice: "round it up". In the code we do a ceil, and that's not rounding.
Comment 3 Tommaso Pecorella 2015-10-31 17:38:14 EDT
Pushed in changeset:   11746:5daa429b3df7
Comment 4 Gustavo J. A. M. Carneiro 2015-11-01 05:56:14 EST
(In reply to Tommaso Pecorella from comment #2)
> Created attachment 2170 [details]
> bug fix
> 
> Dunno if the problem with int128 is still there, but the problem isn't the
> int128.
> 
> The issue is that the function is rounding numbers to the highest fraction,
> and not to the nearest. The standard is quite explicit on this:
>      -    compute the expression 16*(T/(C*(2^b))-1), which may not be a
>           integer, and round it up.  This results in the value for 'a'
> 
> Notice: "round it up". In the code we do a ceil, and that's not rounding.

Not sure I agree.  Doesn't ceil() precisely mean "rounding up"?  In [1], ceil(x) is described as "Rounds x upward".

[1] http://www.cplusplus.com/reference/cmath/ceil/