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

feat: use Math for double::{ln,log2,log10} on JS #1489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tonyfettes
Copy link
Contributor

Follow up of #1485 .

This PR use Math library on JS backend to implement:

  • Double::ln
  • Double::log2
  • Double::log10

TODO: 15.0.log10() is now inconsistent between JS & Other platforms.

Copy link

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

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

Here are three observations from the provided git diff output:

  1. Inconsistent Example in log10 Test:

    • In the log10 test, the line // inspect!(15.0.log10(), content="1.1760912590556811") is commented out. This might be intentional for debugging or testing purposes, but it could also be an oversight. If this test case is valid, it should be uncommented to ensure it runs as part of the test suite. If it's no longer needed, it should be removed to avoid confusion.
  2. Potential Typo in ln Function Documentation:

    • In the documentation for the ln function, the example shows inspect!(1.0.ln(), content="0"). However, the natural logarithm of 1.0 is actually 0.0, not 0. This might be a typo or a formatting issue. The expected output should likely be "0.0" to match the precision of the other examples.
  3. Inconsistent Naming in File Renaming:

    • The file log.mbt is being renamed to log_nonjs.mbt, which is consistent with the naming convention used for other files (e.g., exp_nonjs.mbt). However, the new file log_js.mbt is introduced without a corresponding log.mbt file. This might cause confusion if someone expects a generic log.mbt file to exist. It would be better to ensure that the naming conventions are consistently applied across all files.

These are the key issues I noticed. Let me know if you need further clarification or assistance!

@coveralls
Copy link
Collaborator

coveralls commented Jan 16, 2025

Pull Request Test Coverage Report for Build 4803

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.126%

Totals Coverage Status
Change from base Build 4797: 0.0%
Covered Lines: 4872
Relevant Lines: 5861

💛 - Coveralls

@tonyfettes tonyfettes force-pushed the use-js-math-for-log branch 2 times, most recently from f509bda to e6e0f99 Compare January 16, 2025 12:09
@Kaida-Amethyst
Copy link
Contributor

I noticed that (15.0).log10() exhibits inconsistencies between JS and other platforms. Below is the current implementation:

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

May I propose an improved version with better precision, which I have tested locally and found to perform well?

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

@tonyfettes
Copy link
Contributor Author

@Kaida-Amethyst Thank you for your proposal! Could you please open a PR for that?

@Kaida-Amethyst
Copy link
Contributor

@Kaida-Amethyst Thank you for your proposal! Could you please open a PR for that?

Ok, No probelm 👌🏻 , gonna post a PR soon 😸

@Kaida-Amethyst
Copy link
Contributor

@Kaida-Amethyst Thank you for your proposal! Could you please open a PR for that?

PR link: #1499 😸

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.

3 participants