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

fix unit test failures #139

Open
Patashu opened this issue Nov 24, 2021 · 7 comments
Open

fix unit test failures #139

Patashu opened this issue Nov 24, 2021 · 7 comments
Labels

Comments

@Patashu
Copy link
Owner

Patashu commented Nov 24, 2021

7:08 AM] Razenpok: Hey, Pata
I've ported Number-compatibility tests from C# version to check if Decimal operations produce same values as Number operations. You can check them on tests branch by running npm ci and then npm run test
[7:08 AM] Razenpok: It will run for a bit (takes several seconds on my PC)
image
7:08 AM] Razenpok: And there are a lot of test cases because combinatorial explosion
[7:09 AM] Razenpok: You can check all the source in tests folder
[7:09 AM] Razenpok: Currently, I observe a lot of "Unreachable code" errors and I'm not sure what to do with them
[7:09 AM] Razenpok: Either to disable these test cases or fix the bugs

@Razenpok
Copy link
Collaborator

Razenpok commented Nov 24, 2021

Current issues:

  • Infinity representation in Decimal is weird, as it uses a e9e15 number. This produces incorrect results on unary operations (like exp or log10). Solution: Represent non-finite values in mantissa instead with exponent equals 0 (e.g. { m: Number.POSITIVE_INFINITY, e: 0 }). BreakInfinity.cs already does that.
  • Rounding issues like Decimal.round(1009680).toNumber() is 1009679.9999999999 #52
  • eq_tolerance is not good with small values. For example, new Decimal(0).eq_tolerance(5.323e-47, 1e-10): expected true, actual false
  • A bunch of wrongs with sinh and tanh. E.g. try tanh with 7.23e222 or check test-report.html after running the tests for more examples
  • Divide by zero is not working correctly. new Decimal(1).divide(0) :kekw:
  • Same with pow 0 into anything. Try new Decimal(0).pow(1) and Math.pow(0, 1)
  • Decimal.cmp has unreachable code which is actually reachable when you toss in some value combinations. For example 0, NaN
  • Decimal.cmp produces incorrect values for non-zero value and NaN, try 1 and NaN

@Patashu I need your thoughts about what to do with these
I've also merged tests branch into master, so run tests from there

@Patashu
Copy link
Owner Author

Patashu commented Nov 24, 2021

  • agreed on infinity representation
  • not sure how to fix rounding in general, would appreciate your thoughts. maybe how did you fix it in BreakInifnity.cs?
  • yeah should figure out what the eq_tolerance bug is. bad math or buggy implementation?
  • not surprised some of the trig/hyperbolic trig functions are wrong lmao
  • damn divide by zero is broken? nooo lmao
  • I guess I never bothered to add a special case for pow with 0 as base? should be an easy fix
  • in general I didn't proof the library very hard against nan/inf, so not surprised there are some wacky ones like that, but I think correctness is important enough that it's worth covering all cases even the weird ones

@Razenpok
Copy link
Collaborator

Would like to pick up some of these? I'm going to fix the infinity one at least

@Patashu
Copy link
Owner Author

Patashu commented Nov 24, 2021

next time I'm in the mood to code I'll do whatever you haven't done yet (optimistically it'll be sometime this week)

@Razenpok
Copy link
Collaborator

Razenpok commented Nov 24, 2021

Good news: most of the broken tests were non-finite-values-related *proceeds with fixing infinity on master*
image

@Razenpok
Copy link
Collaborator

I would suggest grabbing hyperbolic functions since I have no idea how they work %)
I'll probably do something with the rounding issue

@Patashu
Copy link
Owner Author

Patashu commented Nov 24, 2021

I got them from slabdrill iirc lol

but I can figure out a good replacement

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