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

XLDateTime calculations for doubles. #299

Closed
wiluite opened this issue Nov 18, 2024 · 6 comments
Closed

XLDateTime calculations for doubles. #299

wiluite opened this issue Nov 18, 2024 · 6 comments
Assignees
Labels
bug Something isn't working resolved This issue has been resolved.

Comments

@wiluite
Copy link

wiluite commented Nov 18, 2024

XLDateTime dt(24472.0);
auto tmo = dt.tm();
std::cout << tmo.tm_year + 1900 << '-' << tmo.tm_mon + 1 << '-' << tmo.tm_mday;

Is this code too problematic? It calculates 0 as 'day of month'.

std::asctime(&tmo) also fails to proceed further, because it probably returns nullptr;

imho:
note, there is a good Howard Hinnant's date library for such things.

@aral-matrix
Copy link
Collaborator

aral-matrix commented Nov 18, 2024

What exactly is your question? Your code example correctly converts an Excel Date to a struct tm - but I think you have a typo? struct tm represents day of the month as 1-31, but the tm_mon field is 0-11, which is just the way C++ implemented it:

https://cplusplus.com/reference/ctime/tm/

@aral-matrix aral-matrix self-assigned this Nov 18, 2024
@aral-matrix aral-matrix added the question Further information is requested label Nov 18, 2024
@wiluite
Copy link
Author

wiluite commented Nov 18, 2024

  1. What excel epoch did you mean while doing tm() method implementation?
  2. How it can be that as a result of the running tm()'s code one can result in, particularly, tm_mday=0?? (not tm_mon)
  3. Do you have restrictions on input doubles? Are there can be just any doubles or not?
  4. Are you sure your tm() is absolutely correct? Yes, I see testXLDateTime.cpp' inputs run with no crash. But... did you use alternative tools to check the results?
  5. My apologies if my connotations of date and time and "serial number" do not have the meaning you intended in the methods of the XLDateTime class.

@aral-matrix
Copy link
Collaborator

What excel epoch did you mean while doing tm() method implementation?

1.0 = 1899-12-31 (0 = 1899-12-30) - please note that MS documentation is contradicting itself every second sentence.

How it can be that as a result of the running tm()'s code one can result in, particularly, tm_mday=0?? (not tm_mon)

Because this module hasn't been reviewed in a while, and MS documentation being as it is, I may have implemented the struct tm epoch 1900-01-01 wrongly - especially since MS Excel date is a clsterfck working around a bug that they made 1900 be considered a leap year when it is not.
I will review the XLDateTime in the next days/weeks & provide a fix.

Do you have restrictions on input doubles? Are there can be just any doubles or not?

Yes, in the current version, no dates before 1900-01-01 are allowed - in doubles that means the value must be >= 1.0.

Are you sure your tm() is absolutely correct?

I am sure the implementation is not correct for all dates. As I said, I haven't gotten around to it yet.

Yes, I see testXLDateTime.cpp' inputs run with no crash. But... did you use alternative tools to check the results?

Yes, but since the buggy epoch implementation was in only one constructor (from double), I didn't notice at the time.

Have some patience please - I have other things to work on as well (the ones that I get paid for ;) - but I'll get around to it. I'll mark this here as a confirmed bug and will not close it until I fix it (multiple, actually).

@aral-matrix aral-matrix added bug Something isn't working and removed question Further information is requested labels Nov 19, 2024
@wiluite
Copy link
Author

wiluite commented Nov 19, 2024

Thanks for detailed reply! ;-) Have your time.

This info could be probably helpful.

    bool is1904 = false; // true or false can be given (openpyxl Python lib or so.) from Excel file

    // Further (logic borrowed from xlrd, Python lib devoted to xls format):
    std::chrono::system_clock::time_point to_chrono_time_point(double d) {
        using ddays = std::chrono::duration<double, date::days::period>;
        if (is1904)
            return date::sys_days{date::January/01/1904} + round<std::chrono::system_clock::duration>(ddays{d});
        else if (d < 60)
            return date::sys_days{date::December/31/1899} + round<std::chrono::system_clock::duration>(ddays{d});
        else
            return date::sys_days{date::December/30/1899} + round<std::chrono::system_clock::duration>(ddays{d});
    }

    #include <date.h>
    using ddays = std::chrono::duration<double, date::days::period>;
    using date::operator<<;

    std::cout << to_chrono_time_point(55.5) << '\n';
    OpenXLSX::XLDateTime dt(55.5);
    std::tm tmo = dt.tm(); 
    assert(std::asctime(&tmo) != nullptr);
    std::cout << std::asctime(&tmo);
    // Equal results:
    // 1900-02-24 12:00:00.000000000
    // Fri Feb 24 12:00:00 1900
 
    std::cout << to_chrono_time_point(1000.1) << '\n';
    tmo = OpenXLSX::XLDateTime(1000.1).tm(); 
    assert(std::asctime(&tmo) != nullptr);
    std::cout << std::asctime(&tmo) << '\n'; 
    // Equal results:
    // 1902-09-26 02:24:00.000000000
    // Fri Sep 26 02:24:00 1902

    std::cout << to_chrono_time_point(1000.55) << '\n';
    tmo = OpenXLSX::XLDateTime(1000.55).tm(); 
    assert(std::asctime(&tmo) != nullptr);
    std::cout << std::asctime(&tmo) << '\n';
    // 1902-09-26 13:12:00.000000000
    // Assertion failed. 

@aral-matrix
Copy link
Collaborator

Hi @wiluite - I just patched XLDateTime, hopefully fixing the issues now - refer 107ad2b for the change.

The bug I had introduced in the past was because I didn't realize LibreOffice (which I use for testing) chose a different solution for Excel Date format bug where it assumes that 1900 is a leap year:

In Excel, if you enter "60", and format as date, you would get "29 February 1900", a day that did not exist in history - and for "61" you would get "1 March 1900". So between the very real days that follow upon each other: 1900-02-28 and 1900-03-01, in Excel, a date diff would yield "2 days", when actually it is only 1 day difference.

LibreOffice "fixed" this by moving the epoch back by 1 day, to start on 1899-12-30T00:00:00. Being unaware of this, I assumed the Excel epoch was also the same, and documentation was wrong. When fixing what I thought was "correct" date displays for the first two months of the epoch - using LibreOffice as control - I broke the correct code (IIRC) from before.

Please have a look and test & let me know if I can close this issue. Thank you for reporting it!

@aral-matrix aral-matrix added the resolved This issue has been resolved. label Dec 9, 2024
@aral-matrix
Copy link
Collaborator

Patch has been merged into main branch. Closing as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved This issue has been resolved.
Projects
None yet
Development

No branches or pull requests

2 participants