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

better trigonometric functions' implementation #1559

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Kaida-Amethyst
Copy link
Contributor

@Kaida-Amethyst Kaida-Amethyst commented Jan 23, 2025

This PR aims to address the issue raised in #1488, where inconsistencies in trigonometric functions were identified. The current implementation shows deviations from JavaScript V8 and glibc, which could lead to precision-related issues in the future.

To resolve this, I have implemented custom versions of sin, cos, and tan based on the well-established fdlibm library. These implementations have undergone extensive testing in the Moonbit-Math repository, demonstrating high consistency with JavaScript V8's results.

References:

update in 01/31/25, add asin, acos, atan, atan2.

@coveralls
Copy link
Collaborator

coveralls commented Jan 23, 2025

Pull Request Test Coverage Report for Build 5152

Details

  • 168 of 220 (76.36%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 85.385%

Changes Missing Coverage Covered Lines Changed/Added Lines %
math/trigonometric.mbt 0 7 0.0%
double/trig_nonjs.mbt 168 213 78.87%
Totals Coverage Status
Change from base Build 5143: -0.5%
Covered Lines: 5375
Relevant Lines: 6295

💛 - Coveralls

@Lampese Lampese self-requested a review January 23, 2025 09:14
@Lampese Lampese self-assigned this Jan 23, 2025
@Lampese
Copy link
Collaborator

Lampese commented Jan 23, 2025

  • sin
  • cos
  • tan
  • atan
  • asin
  • acos
  • atan2
  • move algorithm implements to double
  • move algorithm implements to float
  • add js api
  • add test result to ensure consistency

@@ -0,0 +1,770 @@
// Copyright 2025 International Digital Economy Academy

Choose a reason for hiding this comment

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

Note that the copyright for fdlibm explicitly requires that the copyright notice should be preserved.

/*
 * ====================================================
 * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
 *
 * Developed at SunSoft, a Sun Microsystems, Inc. business.
 * Permission to use, copy, modify, and distribute this
 * software is freely granted, provided that this notice 
 * is preserved.
 * ====================================================
 */

It would be better that the copyright header be reserved for translated functions,
just like what rust do:
https://github.com/rust-lang/libm/blob/a89add35ebda33250cfeec0fdd942ea43a758120/src/math/acos.rs#L4-L9

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you very much! This is a very, very helpful point. We'll go back to work when we get back from the Chinese New Year break.

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.

5 participants