-
Notifications
You must be signed in to change notification settings - Fork 385
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
- #338
- #338
Conversation
var allowedDiff = precision || 1e-11; | ||
return this.within(obj - allowedDiff, obj + allowedDiff); | ||
var ok = Math.abs(1 - this.__flags.object / expected) / Number.EPSILON < MATH_EXP_ERROR + 8; | ||
this.assert(ok, 'expected #{this} to be almost equal ' + expected, 'expected #{this} to not be almost equal' + expected); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not ideal, I do not know how to access private "this.__flags.object" and I did not test it well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this.flags('object')
might work better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb , I fixed by using this.within
@ljharb , OK, what do you think now? |
if (abs < BINARY_32_MIN_VALUE) { | ||
return sign * roundTiesToEven(abs / BINARY_32_MIN_VALUE / BINARY_32_EPSILON) * BINARY_32_MIN_VALUE * BINARY_32_EPSILON; | ||
} | ||
// Veltkamp's splitting (?) | ||
var a = (1 + BINARY_32_EPSILON / Number.EPSILON) * abs; | ||
var result = a - (a - abs); | ||
if (result > BINARY_32_MAX_VALUE || numberIsNaN(result)) { | ||
if (result > BINARY_32_MAX_VALUE || Number.isNaN(result)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change - it's there intentionally, so that the ordering of shims doesn't matter. Same everywhere it used to say numberIsNaN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, result !== result
is better, because it has no dependency on both Number.isNaN
and numberIsNaN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I'd prefer numberIsNaN
so that the NaN checks can all be routed through the same code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I followed this rule
The tests look good, and fail as expected on Chrome 42, node 0.8/0.10/0.11/0.12, and iojs 1.7. |
return (a - b) / (Math.exp(x) + Math.exp(-x)); | ||
var a = Math.expm1(2 * x); | ||
var b = Math.expm1(2 * x) + 2; | ||
return a === b ? 1 : a / b; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what values is a === b && a / b !== 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Infinity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh snap again, good call. Is this faster enough that it's worth the far less clear version, than the previous which had explicit returns for ± Infinity
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no,
if (a === Infinity) { return 1; }
could be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping the explicit return checks for ± Infinity are a good idea - probably also the initial NaN
and 0
checks too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb , I cannot understand how explicit checks can help, I want to have number of "if" branches as small as possible. But I have replaced ternary operator with more readable if (a === Infinity && b === Infinity) { ... }
.
@@ -551,7 +565,11 @@ describe('Math', function () { | |||
expect(Math.sinh(-Infinity)).to.equal(-Infinity); | |||
expect(Math.sinh(-5)).to.almostEqual(-74.20321057778875); | |||
expect(Math.sinh(2)).to.almostEqual(3.6268604078470186); | |||
expect(Math.sinh(-2e-17)).to.equal(-2e-17); | |||
expect(Math.sinh(-2e-17)).to.almostEqual(-2e-17); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which browsers is Math.sinh(-2e-17)
not precisely equal to -2e-17
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb in a browser of 2150 year, which have Number.EPSILON < 2e-17 * 2e-17 / 3
Thanks, this is looking great. I'll test it in a number of browsers before merging it. |
if (b === Infinity) { return -1; } | ||
return (a - b) / (Math.exp(x) + Math.exp(-x)); | ||
var a = Math.expm1(2 * x); | ||
var b = Math.expm1(2 * x) + 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why am I calculating Math.expm1(2 * x)
twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, good question
any chance to finish this? |
Absolutely - it'll need to be freshly rebased on top of master, and then I'll do a fresh round of browser testing - but I don't think it needs much more work. |
|
@Yaffle I'd love to bring this PR in if you want to freshly rebase it - it's been about 10 months :-) |
~2 years |
@Yaffle yes, it's been 1 year since you closed the PR and deleted the branch without rebasing it, and since nobody else seems to understand this topic as well as you do, it will basically never be fixed. I'd be delighted if you reopened the PR and rebased the branch, then it could get in very soon. |
I have put all implementations of not precise ES6 Math functions to the comment at |
Yes, thank you, but a dump of implementations doesn't help me change anything. What is helpful is a set of test cases, justified with evidence (browser results, spec steps, etc), and then also an implementation if a good one isn't apparent. |
|
Thank you, that is much more helpful! |
I'm unable to reproduce the |
@ljharb , the test for Line 2083 in ec76d49
|
thanks, i'll update that. |
@ljharb, thanks |
k, should be all set. |
Fixes #334.