Skip to content

Conversation

@tompng
Copy link
Member

@tompng tompng commented May 5, 2025

Precision assertion in BigMath test is missing.
BigMath uses BigDecimal.double_fig (16 in my env), so the calculated precision is normally >=16.
We need to test with precision several times larger than 16 to ensure that the result is really precise enough.

This assertion will be helpful when adding new methods to BigMath (example: BigMath.tan)

Precision check

Assume BigMath.func(v, 2 * prec) to be the expected result which is precise enough and calculates the difference.
assert_fixed_point_precision is for sin and cos. assert_relative_precision is for other methods.

sin and cos

I think BigMath.sin(x), BigMath.cos(x) with large x is not precise enough. This test case is not added in this pull request.

BigMath.sin(BigDecimal('1e50'), 100) - BigMath.sin(BigDecimal('1e50'), 200)
# => -0.7388355386-67 # precision should be >=100 but is 67

@tompng tompng force-pushed the add_precision_assertion branch from 0b138e4 to c1dfce8 Compare May 13, 2025 12:39
@tompng tompng force-pushed the add_precision_assertion branch from c1dfce8 to 395fb3b Compare May 13, 2025 12:53
precision = -(value / expected - 1).exponent
assert(value != expected, "Unable to estimate precision for exact value")
assert(precision >= n, "Precision is not enough: #{precision} < #{n}")
end
Copy link
Member

Choose a reason for hiding this comment

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

These two assertions seem useful in test_bigdecimal.rb too. Why not relocate them to helper.rb?

include TestBigDecimalBase
include BigMath
N = 20
N_LARGE = 100
Copy link
Member

Choose a reason for hiding this comment

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

This is just a comment to share my concern regarding the use of BigDecimal on 64-bit architectures. Each BigDecimal digit element can hold 19 decimal digits, meaning that 100 would require 6 BigDecimal digits. We should consider if 6 BigDecimal digits is truly a large number.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update the precision assertion to use [50, 100, 150]
Trying multiple precisions will increase the probability of detecting mis-calculation.

def assert_relative_precision(n = N_LARGE)
value = yield(n)
expected = yield(2 * n)
precision = -(value / expected - 1).exponent
Copy link
Member

Choose a reason for hiding this comment

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

Don’t you have to specify the quotient precision explicitly in the division?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to value.div(expected, 2 * n)

@mrkn mrkn merged commit 07facd4 into ruby:master May 16, 2025
69 of 78 checks passed
@tompng tompng deleted the add_precision_assertion branch May 16, 2025 16:51
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.

2 participants