ssf - time rounding for seconds #3006

davidtamaki · 2023-10-10T10:22:37Z

davidtamaki commented

2023-10-10 10:22:37 +00:00

This fixes the edge case where microseconds round a date/time with 59 seconds to the next minute.

before:

>  ssf.format('h:mm:ss', 0.999994)
'23:59:59'
>  ssf.format('h:mm:ss', 0.999995)
'23:59:00'

after:

> ssf.format('h:mm:ss', 0.999994)
'23:59:59'
> ssf.format('h:mm:ss', 0.999995)
'0:00:00'

Matches what we see in sheets:

I looked in to updating this here with the following logic, however this will keep seconds at 59 instead of rounding up through the larger timeframes (minutes, hours, etc):

ss = (tt)*(val.S + val.u);
ss = (ss >=59.5 && ss < 60) ? 59 : Math.round(ss)

Also fixes: https://github.com/SheetJS/ssf/issues/95

This fixes the edge case where microseconds round a date/time with 59 seconds to the next minute. before: ``` > ssf.format('h:mm:ss', 0.999994) '23:59:59' > ssf.format('h:mm:ss', 0.999995) '23:59:00' ``` after: ``` > ssf.format('h:mm:ss', 0.999994) '23:59:59' > ssf.format('h:mm:ss', 0.999995) '0:00:00' ``` Matches what we see in sheets: ![image](/attachments/645eaf32-565d-440f-9a2e-efd482b3c849) I looked in to updating this [here](https://git.sheetjs.com/sheetjs/sheetjs/src/branch/master/packages/ssf/ssf.js#L324) with the following logic, however this will keep seconds at 59 instead of rounding up through the larger timeframes (minutes, hours, etc): ``` ss = (tt)*(val.S + val.u); ss = (ss >=59.5 && ss < 60) ? 59 : Math.round(ss) ``` Also fixes: https://github.com/SheetJS/ssf/issues/95

image.png

4.6 KiB

davidtamaki added 1 commit 2023-10-10 10:22:38 +00:00

74ed41bb95 ssf time rounding for seconds

davidtamaki commented

2023-10-10 10:24:54 +00:00

If this fix looks good - could this get ported to the https://github.com/SheetJS/ssf npm package?

sheetjs commented

2023-10-11 08:21:47 +00:00

Thanks for the report and PR!

The fractional part of v is the time, so you would need to round to the nearest second (1/86400):

	/* Round number up if displaying seconds with no microseconds */
	if (dt && (dt.S + dt.u >= 59.5) && sec && !usec) {
		dt=parse_date_code(Math.round((v - Math.floor(v))*86400)/86400, opts)
	}

It's useful to have a small test harness (see attached file):

var XLSX = require("./"), SSF = require("./packages/ssf");
var wb = XLSX.readFile("i3006.xlsx", {cellNF: true, dense: true});
var data = wb.Sheets.date["!data"]
data.slice(1).forEach((r,R) => {
  var val = data[R+1][0].v;
	r.slice(1).forEach((cell, C) => {
		var fmt = data[0][C+1].v;
		var w = SSF.format(fmt, val);
		if(w != cell.v) console.log(R, C, cell.v, val, fmt, cell.v, w);
	});
});

Logged values are (R-1, C-1, value, format, expected, actual)

There are 18 divergences with the current code, 15 with the original patch, and 11 after correcting the rounding.

Some of the issues have to do with rounding when no seconds or microseconds are specified (h:mm). Others relate to treatment of absolute hours in the presence of date specifiers.

We'll take a closer look. If you have any other rounding cases, we'll throw them into the test.

Thanks for the report and PR! The fractional part of `v` is the time, so you would need to round to the nearest second (1/86400): ```js /* Round number up if displaying seconds with no microseconds */ if (dt && (dt.S + dt.u >= 59.5) && sec && !usec) { dt=parse_date_code(Math.round((v - Math.floor(v))*86400)/86400, opts) } ``` It's useful to have a small test harness (see attached file): ```js var XLSX = require("./"), SSF = require("./packages/ssf"); var wb = XLSX.readFile("i3006.xlsx", {cellNF: true, dense: true}); var data = wb.Sheets.date["!data"] data.slice(1).forEach((r,R) => { var val = data[R+1][0].v; r.slice(1).forEach((cell, C) => { var fmt = data[0][C+1].v; var w = SSF.format(fmt, val); if(w != cell.v) console.log(R, C, cell.v, val, fmt, cell.v, w); }); }); ``` Logged values are (R-1, C-1, value, format, expected, actual) There are 18 divergences with the current code, 15 with the original patch, and 11 after correcting the rounding. Some of the issues have to do with rounding when no seconds or microseconds are specified (`h:mm`). Others relate to treatment of absolute hours in the presence of date specifiers. We'll take a closer look. If you have any other rounding cases, we'll throw them into the test.

i3006.xlsx

10 KiB

davidtamaki added 1 commit 2023-10-12 09:30:14 +00:00

c2d38f2e07 fix ssf h:mm rounding

davidtamaki commented

2023-10-12 09:53:49 +00:00

Thanks for the feedback!

I added another check which resolves 2 of the divergences of that test relating to the h:mm format.

Of the remaining 9 divergences, I'm wondering if this may be an issue with XLSX.readFile rather than SSF.format. I've copied the sheet to here and highlighted the differences.

The remaining 9 divergences match the results we see in that spreadsheet (with the caveat that Google sheets apparently starts time 2 days before excel at 1899-12-30 0:00:00 vs 1900-01-00 0:00:00, but the times do match)

One of the expected results is also off, but it doesn't appear to be related to rounding issues:

(index)	Values
R	3
C	17
val	0.00556
fmt	'yyyy-mm-dd [h]:mm:ss'
expected	'1900-01-00 21:08:00'
actual	'1900-01-00 0:08:00'

Thanks for the feedback! I added another check which resolves 2 of the divergences of that test relating to the `h:mm` format. Of the remaining 9 divergences, I'm wondering if this may be an issue with `XLSX.readFile` rather than `SSF.format`. I've copied the sheet to [here](https://docs.google.com/spreadsheets/d/1EKk_FLBePL0gWV_aU4_TggLY6xge9ylFr3hkCwdrCE0/edit#gid=0) and highlighted the differences. The remaining 9 divergences match the results we see in that spreadsheet (with the caveat that Google sheets apparently starts time 2 days before excel at `1899-12-30 0:00:00` vs `1900-01-00 0:00:00`, but the times do match) One of the expected results is also off, but it doesn't appear to be related to rounding issues: | (index) | Values | | :- | :-: | | R | 3 | | C | 17 | | val | 0.00556 | | fmt | 'yyyy-mm-dd [h]:mm:ss' | | expected | '1900-01-00 21:08:00' | | actual | '1900-01-00 0:08:00' |

davidtamaki commented

2023-10-18 09:44:17 +00:00

@sheetjs is there anything else I need to do on this PR? I'd like to get this change merged in if there are not any further issues / tests to run. Thanks!

sheetjs merged commit 59eec8fd5f into master

2023-10-23 17:21:52 +00:00

sheetjs deleted branch davidtamaki/ssf-duration-rounding

2023-10-23 17:21:52 +00:00

sheetjs commented

2023-10-23 18:27:31 +00:00

yyyy-mm-dd [h]:mm:ss is definitely bugged in Excel.

The absolute hours field is clearly wrong given the values (since none of the values exceed 1, the absolute hours should not exceed 24).

PS: XLSX.readFile does not actually format the values in this test. The test file uses the TEXT function, ensuring that Excel serializes the formatted text. For example, in the attached file, C4 is stored as follows:

<c r="C4" t="str">
  <f t="shared" si="1"/>
  <v>1900-01-00 588:14:24</v>
</c>

`yyyy-mm-dd [h]:mm:ss` is definitely bugged in Excel. ![abs-hrs-with-ymd.png](/attachments/10081573-1ae9-48cf-b20e-5d441f3c44bc) The absolute hours field is clearly wrong given the values (since none of the values exceed 1, the absolute hours should not exceed 24). PS: `XLSX.readFile` does not actually format the values in this test. The test file uses the `TEXT` function, ensuring that Excel serializes the formatted text. For example, in the attached file, C4 is stored as follows: ```xml <c r="C4" t="str"> <f t="shared" si="1"/> <v>1900-01-00 588:14:24</v> </c> ```

excel-bad-abs-hours.xlsx

9.3 KiB

abs-hrs-with-ymd.png

43 KiB

sheetjs commented

2023-10-24 01:29:49 +00:00

We ended up having to shift the rounding logic elsewhere. To summarize, Excel enforces 15 decimal digits precision.
#3003 has a more detailed example with numbers.

After more precise rounding rules, a few of the disabled tests also passed 🥳

Version 0.11.3 has been released.

Standalone script

URL: https://cdn.sheetjs.com/ssf-0.11.3/ssf.js

Adding to a page:

<script src="https://cdn.sheetjs.com/ssf-0.11.3/ssf.js"></script>

Live Demo: https://jsfiddle.net/vz0h9Lxd/

NodeJS package

URL: https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz

Installation:

npm install --save https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz

Package installs to module name ssf so it should be a drop-in replacement for the legacy distribution point.

We ended up having to shift the rounding logic elsewhere. To summarize, Excel enforces 15 decimal digits precision. https://git.sheetjs.com/sheetjs/sheetjs/issues/3003 has a more detailed example with numbers. After more precise rounding rules, a few of the disabled tests also passed 🥳 Version `0.11.3` has been released. #### Standalone script URL: https://cdn.sheetjs.com/ssf-0.11.3/ssf.js Adding to a page: ```html <script src="https://cdn.sheetjs.com/ssf-0.11.3/ssf.js"></script> ``` Live Demo: https://jsfiddle.net/vz0h9Lxd/ #### NodeJS package URL: https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz Installation: ```bash npm install --save https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz ``` Package installs to module name `ssf` so it should be a drop-in replacement for the legacy distribution point.

🎉 1

davidtamaki commented

2023-11-06 19:38:46 +00:00

Thanks @sheetjs !

Are there any plans to publish 0.11.3 to npm? Or should we no longer expect any updates to that repo? Ideally we would prefer to use npm as a distribution point.

Thanks @sheetjs ! Are there any plans to publish `0.11.3` to npm? Or should we no longer expect any updates to that repo? Ideally we would prefer to use npm as a distribution point.

sheetjs commented

2023-11-06 21:00:41 +00:00

The SheetJS CDN https://cdn.sheetjs.com/ is the authoritative source for SheetJS modules.

Legacy distribution points (including the SheetJS/sheetjs Git repositories on GitHub and the ssf package on npmjs.com) will not be receiving updates.

Vendoring

The dependency tarball can be "vendored" in your repository:

Download tarball to a vendor subfolder:

mkdir -p vendor
curl -o vendor/ssf-0.11.3.tgz https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz

Add to your source repository:

git add vendor/ssf-0.11.3.tgz

Install the tarball. The file: prefix tells the NPM client that the specified name is a tarball:

npm install --save file:vendor/ssf-0.11.3.tgz

In general, it is strongly recommended to use a proxying registry like Verdaccio

Nested Dependency

If ssf is a dependency of a dependency, you can add an override to package.json:

{
  "overrides": {
    "ssf": "https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz"
  }
}

This can be combined with the "Vendoring" approach:

{
  "overrides": {
    "ssf": "file:vendor/ssf-0.11.3.tgz"
  }
}

The SheetJS CDN https://cdn.sheetjs.com/ is the authoritative source for SheetJS modules. Legacy distribution points (including the `SheetJS/sheetjs` Git repositories on GitHub and the `ssf` package on npmjs.com) will not be receiving updates. #### Vendoring The dependency tarball can be "vendored" in your repository: 1) Download tarball to a `vendor` subfolder: ```bash mkdir -p vendor curl -o vendor/ssf-0.11.3.tgz https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz ``` 2) Add to your source repository: ```bash git add vendor/ssf-0.11.3.tgz ``` 3) Install the tarball. The `file:` prefix tells the NPM client that the specified name is a tarball: ```bash npm install --save file:vendor/ssf-0.11.3.tgz ``` In general, it is strongly recommended to use a proxying registry like [Verdaccio](https://verdaccio.org/) #### Nested Dependency If `ssf` is a dependency of a dependency, you can add an override to `package.json`: ```json { "overrides": { "ssf": "https://cdn.sheetjs.com/ssf-0.11.3/ssf-0.11.3.tgz" } } ``` This can be combined with the "Vendoring" approach: ```json { "overrides": { "ssf": "file:vendor/ssf-0.11.3.tgz" } } ```

👍 1

davidtamaki commented

2023-11-07 14:36:59 +00:00

Thanks for the clarification!

Sign in to join this conversation.

No reviewers