Skip to content

Conversation

@tompng
Copy link
Member

@tompng tompng commented May 11, 2025

BigMath.sin and cos was returning > 1 or <-1 value in few cases.

irb(main):001> BigMath.sin(BigMath::PI(30) / 2, 30)
=> 0.1000000000000000000000000000000000000000000000099061665879329355e1
irb(main):002> BigMath.cos(BigMath::PI(30), 30)
=> -0.1000000000000000000000000000000000000000000004381047173448900999e1

The precision is correct (error < 1e-44), but sin and cos is often assumed to be within -1..1.

sin = BigMath.sin(x)
cos = (1 - sin**2) ** 0.5 # assume sin**2 to be <= 1, but it was not ensured

@mrkn mrkn force-pushed the sin_cos_range_fix branch from f48f4fe to 511c216 Compare May 15, 2025 02:30
@mrkn
Copy link
Member

mrkn commented May 15, 2025

@tompng, it may be advisable to apply a provisional adjustment so the result falls within each function’s valid range.
We should be mindful of loss of precision when $|x|$ approaches to $\pi k/2$ for sin and $k\pi$ for cos, respectively.
Could you investigate the extent of the loss of precision in these regions?

@mrkn mrkn modified the milestones: v3.2, v3.3 May 29, 2025
@tompng tompng force-pushed the sin_cos_range_fix branch from 511c216 to c39cfc6 Compare June 12, 2025 14:44
@tompng
Copy link
Member Author

tompng commented Jun 12, 2025

There is no loss of accuracy. Error decreases in every case.

if master_branch_sin(x).abs <= 1
  # no change, no accuracy loss
  return master_branch_sin(x)
elsif master_branch_sin(x) > 1
  return 1
  # true_sin(x) <= this_pull_req_sin(x) == 1 <= master_branch_sin(x), so the error always decrease
elsif master_branch_sin(x) < -1
  return -1
  # master_branch_sin(x) < this_pull_req_sin(x) == 1 <= true_sin(x)
end

@mrkn mrkn force-pushed the sin_cos_range_fix branch from c39cfc6 to 8c2359c Compare June 17, 2025 14:21
@mrkn mrkn merged commit eaa9d8a into ruby:master Jun 17, 2025
78 checks passed
@tompng tompng deleted the sin_cos_range_fix branch June 17, 2025 15:17
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