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

Conversion of float/double in CSVWriter is broken #188

Closed
jleugeri opened this issue Nov 22, 2021 · 1 comment
Closed

Conversion of float/double in CSVWriter is broken #188

jleugeri opened this issue Nov 22, 2021 · 1 comment
Assignees
Labels

Comments

@jleugeri
Copy link

Hi, I noticed a critical bug:
when trying to save values close to 10^n for some n (e.g. 1000.0, 1000000.0), the leading decimal is missing in the exported .csv file (e.g. the string is saved as "000.0" or "000000.0", respectively).
I traced this behaviour back to the way the digits are serialized in csv_writer.hpp.

The problem comes from the fact that the log(x)/log(10) is an imprecise method to estimate the number of digits of x, as speculated e.g. in this question on stackoverflow.

To verify that this is indeed the problem, I tested the following minimal (non-)working example (I'm compiling with g++ (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 on WSL, but the behaviour should be the same regardless of compiler):

#include <iostream>
#include <math.h>

int main() {
    double d = 1000.0;
    double integral_part;
    double fractional_part = std::abs(std::modf(d, &integral_part));
    integral_part = std::abs(integral_part);

    int n_digits = (int)(std::log(integral_part) / std::log(10));
    std::cout << "d: " << d << ", integral_part: " << integral_part << ", fractional_part: " << fractional_part << ", log(integral_part): " << std::log(integral_part) << ", log(10): " << std::log(10) << ", n_digits: "<< n_digits << std::endl;
    
    // Produces:
    // d: 1000, integral_part: 1000, fractional_part: 0, log(integral_part): 6.90776, log(10): 2.30259, n_digits: 2

    // Should produce:  ------------------------------------------------------------------------------------------------------v
    // d: 1000, integral_part: 1000, fractional_part: 0, log(integral_part): 6.90775527898, log(10): 2.30258509299, n_digits: 3
    
    return 0;
}

I believe this is a breaking error, because right now it may invalidate data in a way that is very hard to spot (at least it took me a long time to find). If the fix means taking a slower algorithm for serialization, I'd be fine with that, because correctness > speed (for me at least).
Best regards and thanks for your work on this library!

@vincentlaucsb vincentlaucsb self-assigned this May 17, 2022
@vincentlaucsb
Copy link
Owner

Should be fixed in the latest release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants