fix rounding issues with microseconds calculation #4476
Conversation
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Co-authored-by: Benjamin Gehrels <github@gehrels.info> Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
|
Do not replace explicitdate.xlsx. The tests involving it still need to work after your change, and, as you can see, they don't. If you have a different spreadsheet which demonstrates a problem with the existing code and which your PR fixes, please add that spreadsheet to your PR and test using it. |
|
I am unsure of the purpose of: $microseconds = (int) round($microseconds, -2);If, for example, we start with a dayPart value of 0.1234567, microseconds is initially 658880. Then you apply this magic rounding to change it to 658900. Please explain why. |
|
Actually, the problem is not with the microseconds calculation. The problem is that Php is not generating the full value for a float when casting to string. The calculation yields a result of Subject to the usual warning that floating-point arithmetic is imperfect (and this is exacerbated by the very small numbers we're interested in), it is possible that the Writer can do better. I will look into it. |
Fix PHPOffice#3899. Supersedes PR PHPOffice#4476, which will be changed to draft status and closed if this PR is merged. A standard cast from float to string in PHP can drop trailing decimal positions. This can lead to problems above and beyond the usual problems associated with floating point. See the superseded PR for a more complete explanation. `StringHelper::convertToString` is changed for how it handles floats. It will now do separate casts for the whole and decimal parts, and then combine the results. This affects `Cell::getValueString` and `Cell::getCalculatedValueString`. Xlsx Writer will now invoke `convertToString` before writing a float to Xml. Ods Writer already uses `getValueString`, so no change is needed there. Xls Writer writes its float values in binary, so no change is needed there. Tests are added for all 3 writers. Aside from fixing some problems, it might appear that this change introduces some new problems. For instance, setting a cell to `12345.6789` will now result in `12345.67890000000079` in the Xml. This difference is an illusion, merely a consequence of floating point rounding. If you run the following check under PhpUnit, it will pass: ```php self::assertSame(12345.6789, 12345.67890000000079); ```
|
This PR is superseded by #4479. It is now converted to draft status, and will be closed if 4479 is merged. |
|
PR 4479 is implemented. This can be closed. |
2 participants
This is:
Checklist:
Why this change is needed?
A bug was reported with rounding issues (#3899) that is said to be connected to #3677 and I also experience a failing test downstream, when a stored time of 13:37 is interpreted as 13:36 due to the same issues.
Modifications to the rounding strategies can vastly improve the situation by successfully testing existing cases as well as satisfying new tests, based on the failure I experience, as well as the test provided in #3899 (I included @BGehrels as co-author in that respective commits).
I did not play around much with the GMP extension, but this might also be an alternative for more precise calculation? With this here we lose the .1000000 part of a second, which however already suffers to inaccuracies, hence it is acceptable.
There is one failure I receive, though I suppose this is due to how I modified the test excel sheet. Tried with both LibreOffice as well as some MS Office Online thing, but it does not make a difference. The only (knowing) change i did was adding the time
13:37to cellF3and formatted it to show hours and minutes.