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

A better log10 implementation #1499

Merged

Conversation

Kaida-Amethyst
Copy link
Contributor

The original log10 implementation:

// current version
pub fn log10(self : Double) -> Double {
  ln(self) / ln10
}

has precision issues. For example, log10(1000) yields 2.9999999999999996 instead of the expected 3.

In this PR, I've implemented a more accurate algorithm and added additional test cases:

pub fn log10(self : Double) -> Double {
  if self < 0.0 {
    return not_a_number
  } else if self.is_nan() || self.is_inf() {
    return self
  } else if self == 0.0 {
    return neg_infinity
  }
  let ivln10 = 4.34294481903251816668e-01
  let log10_2hi = 3.01029995663611771306e-01
  let log10_2lo = 3.69423907715893078616e-13
  let (f, e) = frexp(self)
  let (f, e) = if e >= 1 {
    (f * 2.0, (e - 1).to_double())
  } else {
    (f, e.to_double())
  }
  let z = e * log10_2lo + ivln10 * f.ln()
  z + e * log10_2hi
}

The modified algorithm is inspired by fdlibm. Reference: fdlibm/e_log10.c.

@coveralls
Copy link
Collaborator

coveralls commented Jan 17, 2025

Pull Request Test Coverage Report for Build 4822

Details

  • 5 of 8 (62.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 83.087%

Changes Missing Coverage Covered Lines Changed/Added Lines %
double/log.mbt 5 8 62.5%
Totals Coverage Status
Change from base Build 4821: -0.03%
Covered Lines: 4883
Relevant Lines: 5877

💛 - Coveralls

Copy link
Contributor

@tonyfettes tonyfettes left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

peter-jerry-ye-code-review bot commented Jan 17, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Based on the git diff provided, here are the potential issues I've noticed:

  1. Inconsistency in test assertions: In the "log10" test cases, there's a mix of different precision levels in the expected results. For example, 0.1.log10() is expected to be "-1" (integer), while 15.0.log10() is expected to be "1.1760912590556813" (high precision). This inconsistency could lead to unexpected test failures if the actual implementation returns full precision for all cases.

  2. Removal of ln10 constant: The constant ln10 was removed, but it's a commonly used mathematical constant. While the code now uses ivln10 (inverse of ln10) directly, having the original ln10 constant might have been useful for other calculations or for code clarity.

  3. Different comparison style: In the log10 function, some comparisons use == (e.g., self == 0.0) while other parts of the code might benefit from using approximate equality comparisons when dealing with floating-point numbers, as exact equality comparisons can be problematic with floating-point arithmetic.

@bobzhang bobzhang force-pushed the better_log10_implementation branch from fe5d9e8 to 65e2d6b Compare January 18, 2025 02:17
@bobzhang bobzhang enabled auto-merge (squash) January 18, 2025 02:17
@bobzhang bobzhang merged commit 7302fdc into moonbitlang:main Jan 18, 2025
14 checks passed
lynzrand pushed a commit to SyoujyoujiNaiki/moonbit-core that referenced this pull request Jan 22, 2025
@Kaida-Amethyst Kaida-Amethyst deleted the better_log10_implementation branch January 24, 2025 14:24
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.

4 participants