Fix float data conversion to string #1632

Merged
merged 2 commits into from Dec 12, 2012

Projects

None yet

3 participants

@strk
strk commented Dec 6, 2012

Fixes precision digits, closing #430
Also avoids forcing a trailing '.0', closing #1627

Compatibility is still not 100% matching 2.0.x because in two cases:

  • Scientific notation is never used
  • Decimal digits after 16th are not printed

See http://strk.keybit.net/tmp/report_full.txt

Sandro Santilli Fix float data conversion to string
Fixes precision digits, closing #430
Also avoids forcing a trailing '.0', closing #1627
b51b357
@strk
strk commented Dec 6, 2012

NOTE: this commit breaks a test, but I think the test is bogus, as it expects a floating number 1 to be represented with a trailing zero, which is what #1627 was filed to complain about (backward compatibility issue)

@springmeyer
Member

Can you create a C++ test file to go along with this commit to capture all the testing work and prevent regressions?

@strk
strk commented Dec 11, 2012

ok, working on it

@strk
strk commented Dec 11, 2012

One thing: should I expect what the stringstream version gave us or can we upgrade expectance of some of the results ?

For example stringstream did use scientific notation for some values, while the karma version never does.
Also the karma version forces a limit on the number of decimal digits to std::numeric_limits<T>::digits10 + 1

Sandro Santilli Add test for backward compatibility of double to string conversion
The testcase is ready to host more conversion tests but is currently
really only targetting the double-to-string.

refs #430, #1632
9416a57
@strk
strk commented Dec 11, 2012

So I added the test expecting 100% backward compatibility. But the code isn't there yet (would need more overrides)

@strk
strk commented Dec 11, 2012

One of the incompatibilities (number of decimal digits printed) is IMHO due to a bug in boost:
https://svn.boost.org/trac/boost/ticket/7785

We could workaround it by writing our own call_n function, but I dunno if it's worth the trouble.

@strk
strk commented Dec 11, 2012

I don't get how TravisCI doesn't see the test failures, btw, there must be something broken there

@springmeyer
Member

Travis does not run the tests

@springmeyer
Member

@artemp - could you review this?

@artemp
Member
artemp commented Dec 12, 2012

@strk - Thanks for implementing this!

@artemp artemp merged commit e60a3f7 into mapnik:master Dec 12, 2012

1 check passed

Details default The Travis build passed
@artemp
Member
artemp commented Dec 14, 2012

@strk @springmeyer - couple issues :

log10 (0.0) returns -inf
log10 of negative number returns nan

@strk
strk commented Dec 14, 2012

artemp: could you update the testcase with those numbers ? I'll look at them on monday if nobody beats me to it

@artemp
Member
artemp commented Dec 14, 2012

@strk - done in 9453d93

@strk
strk commented Dec 17, 2012

@artemp I tried those number against my offline tester and they don't seem to be giving any problem.
I do see the -inf return, but it's converted to 0 when cast to integer, and I do not get any nan on negative values.
This is with GCC 4.6.3

I see no problem with handling those cases (check for zero and fabs the N in precision function), but being unable to reproduce the problem I'm not the best person to do that.

@artemp
Member
artemp commented Dec 17, 2012

@strk - ok, I'll have a look

@strk
strk commented Dec 17, 2012

@artem I worked on a version also doing scientific notation, so I'll be requesting a new pull shortly, also fixing the inf/nan cases.

The more I work with karma the less I think it's appropriate to perform parametrized formatting. All those compile-time policies don't seem to play nicely with runtime determined policies...

@strk
strk commented Dec 17, 2012

This is the updated report_full.txt with my latest changes (for scientific notation):
http://strk.keybit.net/tmp/report_full.txt

I think we do the 9.000000000000001e-05 better than stringstream as the input number was 0.9e-4

@artemp
Member
artemp commented Dec 17, 2012

@strk - great, btw this what works for me in wkt generator:

template <typename T>
struct wkt_coordinate_policy : karma::real_policies<T>
{
    typedef boost::spirit::karma::real_policies<T> base_type;
    static int floatfield(T n) { return base_type::fmtflags::fixed; }
    static unsigned precision(T n)
    {
        if (n == 0.0) return 0;
        return static_cast<unsigned>(15 - boost::math::trunc(log10(std::fabs(n))));
    }
    template <typename OutputIterator>
    static bool dot(OutputIterator& sink, T n, unsigned precision)
    {
        if (n == 0) return true;
        return base_type::dot(sink, n, precision);
    }
    template <typename OutputIterator>
    static bool fraction_part(OutputIterator& sink, T n
                       , unsigned adjprec, unsigned precision)
    {
        if (n == 0) return true;
        return base_type::fraction_part(sink, n, adjprec, precision);
    }
};
@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this pull request Aug 22, 2013
Sandro Santilli Add test for backward compatibility of double to string conversion
The testcase is ready to host more conversion tests but is currently
really only targetting the double-to-string.

refs #430, #1632
8a0fdf9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment