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

add tests for Math.sumPrecise #4049

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add tests for Math.sumPrecise #4049

wants to merge 4 commits into from

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Apr 10, 2024

The proposal for Math.sumPrecise is now stage 2.7. I've written tests which exercise

  • the basic property descriptor
  • the behavior for iterables, and non-iterable arguments
  • rejection of non-Number values
  • edge cases with -0, NaN, and ±Infinity
  • the actual full-precision summation, using cases I wrote or ran into in my own implementation

I don't have a principled approach for what cases to include for the last bullet point. If implementations submit more, I'm inclined to include anything that actually tripped up any implementation in a way which was not covered by an existing test.

@bakkot bakkot requested a review from a team as a code owner April 10, 2024 22:19
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matches the tests in my polyfill.

@ljharb ljharb requested a review from a team April 10, 2024 22:20
@bakkot bakkot requested a review from a team as a code owner April 10, 2024 22:21
@ptomato
Copy link
Contributor

ptomato commented Apr 12, 2024

I looked through the tests at a high level and they seem correct.

I'm working on documentation for how to write a testing plan and this proposal seemed the right size to try it out on, so I've written that up in #4054. I'd be interested in your opinion; too detailed, not detailed enough?

@linusg
Copy link
Member

linusg commented Apr 18, 2024

I'd also like to see a test case for whatever input values trigger this (non-standard?) RangeError in the polyfill: https://github.com/tc39/proposal-math-sum/blob/3c56f3df9f89d862515617c7e421f51b28bb71f8/polyfill/polyfill.mjs#L88

@bakkot
Copy link
Contributor Author

bakkot commented Apr 18, 2024

That RangeError is standard (step 7.b.ii.1), but is unreachable without providing 2**53 inputs (since each input can increment the overflow by at most one), which is impractical to test.

(Strictly speaking I'm checking for a slightly different thing - the polyfill can potentially work past the point where the spec errors - but like the note says, it is not expected to be reached in practice.)

@linusg
Copy link
Member

linusg commented Apr 18, 2024

I see, thanks!

@bakkot
Copy link
Contributor Author

bakkot commented May 14, 2024

I removed the "awaiting author" label because there's no outstanding feedback for me afaik.

@ptomato
Copy link
Contributor

ptomato commented May 15, 2024

Landing this is fine with me. NB, I did not confirm each numerical result in the numerical tests, I'm prepared to trust @bakkot on that.

I haven't looked at whether it covers every item in the testing plan #4054, but in general I don't think that should block PRs from landing.

@bakkot
Copy link
Contributor Author

bakkot commented May 15, 2024

I haven't looked at whether it covers every item in the testing plan #4054, but in general I don't think that should block PRs from landing.

It definitely doesn't, though I think it covers a majority.


You can check the numeric results against Python with the following script if you'd like:

import sys
import re
import math

if len(sys.argv) != 2:
  print('Usage: python check-sum-tests.py /path/to/sumPrecise/sum.js')
  sys.exit(1)

with open(sys.argv[1]) as f:
  file = f.read()

for line in file.splitlines():
  if 'Math.sumPrecise(' not in line:
    continue
  match = re.search("Math\.sumPrecise\(\[([^]]*)\]\), ([^)]*)\)", line)

  vals = [float(x.strip()) for x in match.group(1).split(',')]
  assertedResult = float(match.group(2))
  try:
    actualResult = math.fsum(vals)
  except OverflowError:
    # python's fsum can't handle intermediate overflow
    # we can work around it by summing the large parts seperately,
    # with those parts shrunk by a factor of 64 so the result doesn't overflow
    # as long as there is a reasonable number of items this works fine
    # order doesn't matter because this is fsum
    # and shrinking then growing by a factor of 64 doesn't lose precision because
    # the values are large
    large = [x / 64 for x in vals if abs(x) > 1]
    small = [x for x in vals if abs(x) <= 1]
    actualResult = 64 * math.fsum(large) + math.fsum(small)
  print(actualResult == assertedResult)

Of course, my tests were against an implementation derived from the same logic as fsum, so that won't catch errors between fsum and the spec. But it's at least enough to confirm I didn't make any copy/paste errors. (Also I checked the results against an entirely different implementation and they matched.)

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

Successfully merging this pull request may close these issues.

None yet

5 participants