Move `double` method to func in `math` #1050

qazxcdswe123 · 2024-09-30T07:17:25Z

This pull request focuses on deprecating several mathematical functions in the Double type and moving their implementations to the math module. Key changes include marking functions as deprecated, adding new implementations, and updating tests accordingly.

Deprecation and Implementation Changes:

double/double.mbti: Marked exp, log10, log2, min_normal, and nan as deprecated. [1] [2]
double/exp_js.mbt and double/exp_nonjs.mbt: Added deprecation alerts for exp and provided new implementations. [1] [2]
double/log.mbt: Commented out deprecation for ln and added deprecation alerts for log2 and log10. [1] [2] [3]

New Implementations in math Module:

math/exp_js.mbt and math/exp_nonjs.mbt: Added new implementations for exp function. [1] [2]
math/log.mbt: Added new implementations for ln, log2, and log10 functions.

Test Updates:

double/exp_test.mbt: Removed old tests for exp, log2, log10, and ln. [1] [2]
math/exp_test.mbt: Added new tests for exp, including special values and overflow/underflow cases.
math/log.mbt: Added new tests for log2, log10, and ln.

Other Changes:

math/math.mbti: Added function signatures for exp, ln, log10, and log2.
math/moon.pkg.json: Updated package configuration to include new targets for exp_js.mbt and exp_nonjs.mbt.
Some problems encounterd

double::ln is used by log2 and log10, so a deprecation notice will persist since we also want to deprecate ln
sqrt is implemented as method (and overloaded to support both float and double) and function argument cannot be overloaded, so it's not possible to migrate this.

close #1046

coderabbitai · 2024-09-30T07:17:32Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

peter-jerry-ye-code-review · 2024-09-30T07:17:46Z

Here are three potential issues or improvements I've identified from the git diff output:

Duplicate License Notice:
- In the NOTICE file, the license notice for the exp.mbt file is duplicated for both double/exp.mbt and math/exp.mbt. While this isn't necessarily a problem, it could be streamlined for clarity and consistency.
- Suggestion: Update the NOTICE file to reflect that math/exp.mbt and double/exp.mbt share the same source and license, rather than listing them separately.
Test File Deletion and Addition:
- The test file double/exp_test.mbt was deleted, while new test files math/exp_test.mbt and others were added. This might indicate a refactoring or consolidation of test cases.
- Suggestion: Ensure that all test cases from the deleted double/exp_test.mbt are either moved or replaced in the new test files to maintain coverage.
Code Porting and Normalization:
- In math/exp_nonjs.mbt, the function normalize checks if the absolute value of f is less than @double.min_positive. If so, it multiplies f by a large number and adjusts an exponent. This approach duplicates the functionality of frexp, which is used later in the same file.
- Suggestion: Consider refactoring the code to use frexp directly for normalization, which could simplify the implementation and possibly reduce redundancy.

These suggestions aim to improve clarity, consistency, and efficiency in the codebase.

coveralls · 2024-09-30T07:19:21Z

Pull Request Test Coverage Report for Build 3311

Details

37 of 42 (88.1%) changed or added relevant lines in 2 files are covered.
22 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 82.805%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
math/exp_nonjs.mbt	22	27	81.48%

Files with Coverage Reduction	New Missed Lines	%
double/exp_nonjs.mbt	22	0.0%

Totals
Change from base Build 3301:	-0.4%
Covered Lines:	4103
Relevant Lines:	4955

💛 - Coveralls

bobzhang · 2024-09-30T07:35:54Z

the CI failure looks weird

qazxcdswe123 · 2024-09-30T07:42:44Z

the CI failure looks weird

Because fromjson tojson for record with Option inside changed, so bleeding and current release's compiler behave differently.

See:

core/json/to_json_test.mbt

Line 132 in 7baf756

struct Opt {

hackwaly · 2024-09-30T08:41:56Z

I think it's better to make math.xxx call Double::xxx to provide generic interface. And not just move Double::xxx to math package.

As there may be sqrt func in BigInt, BigDecimal...

// math package

trait Num {
  sqrt() -> Self
}

fn sqrt[T: Num](self: T) -> T {
  self.sqrt()
}

tonyfettes · 2024-10-05T16:20:01Z

Please make sure to update NOTICE file to include updated path for the exp implementation (as per this comment, math/exp_nonjs.mbt).

tonyfettes · 2024-10-06T10:22:51Z

This may or may not be in the scope of this PR, but for consistency, math functions in float shloud be moved into math package as well.

qazxcdswe123 · 2024-10-08T02:48:15Z

This may or may not be in the scope of this PR, but for consistency, math functions in float shloud be moved into math package as well.

Unfortunately unless we bring back Num trait in X and make it generic, it's not possible to overload the function with different argument and same name.

tonyfettes · 2024-10-08T02:54:31Z

Unfortunately unless we bring back Num trait in X and make it generic, it's not possible to overload the function with different argument and same name.

IMO adding f to every Float function looks good enough, like:

lnf
logf
sinf
cosf
...

qazxcdswe123 requested a review from bobzhang September 30, 2024 07:17

qazxcdswe123 marked this pull request as ready for review September 30, 2024 07:17

qazxcdswe123 force-pushed the move-to-math branch 2 times, most recently from 318e716 to 7baf756 Compare September 30, 2024 07:21

qazxcdswe123 force-pushed the move-to-math branch from 7bdac9c to 86310f8 Compare October 8, 2024 01:51

qazxcdswe123 and others added 5 commits October 8, 2024 09:51

refacotr: exp

d76b06c

refactor: log

04c2082

ci: mbti

49f406f

Merge branch 'main' into move-to-math

3f5fb6e

Merge branch 'main' into move-to-math

ebca879

qazxcdswe123 force-pushed the move-to-math branch from 86310f8 to ebca879 Compare October 8, 2024 01:52

chore: update NOTICE

d41bae8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `double` method to func in `math` #1050

Move `double` method to func in `math` #1050

qazxcdswe123 commented Sep 30, 2024 •

edited by bobzhang

Loading

coderabbitai bot commented Sep 30, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

peter-jerry-ye-code-review bot commented Sep 30, 2024 •

edited

Loading

coveralls commented Sep 30, 2024 •

edited

Loading

bobzhang commented Sep 30, 2024

qazxcdswe123 commented Sep 30, 2024 •

edited

Loading

hackwaly commented Sep 30, 2024 •

edited

Loading

tonyfettes commented Oct 5, 2024

tonyfettes commented Oct 6, 2024

qazxcdswe123 commented Oct 8, 2024 •

edited

Loading

tonyfettes commented Oct 8, 2024 •

edited

Loading

Move double method to func in math #1050

Are you sure you want to change the base?

Move double method to func in math #1050

Conversation

qazxcdswe123 commented Sep 30, 2024 • edited by bobzhang Loading

coderabbitai bot commented Sep 30, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

peter-jerry-ye-code-review bot commented Sep 30, 2024 • edited Loading

coveralls commented Sep 30, 2024 • edited Loading

Pull Request Test Coverage Report for Build 3311

Details

💛 - Coveralls

bobzhang commented Sep 30, 2024

qazxcdswe123 commented Sep 30, 2024 • edited Loading

hackwaly commented Sep 30, 2024 • edited Loading

tonyfettes commented Oct 5, 2024

tonyfettes commented Oct 6, 2024

qazxcdswe123 commented Oct 8, 2024 • edited Loading

tonyfettes commented Oct 8, 2024 • edited Loading

Move `double` method to func in `math` #1050

Move `double` method to func in `math` #1050

qazxcdswe123 commented Sep 30, 2024 •

edited by bobzhang

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

peter-jerry-ye-code-review bot commented Sep 30, 2024 •

edited

Loading

coveralls commented Sep 30, 2024 •

edited

Loading

qazxcdswe123 commented Sep 30, 2024 •

edited

Loading

hackwaly commented Sep 30, 2024 •

edited

Loading

qazxcdswe123 commented Oct 8, 2024 •

edited

Loading

tonyfettes commented Oct 8, 2024 •

edited

Loading