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: implement Exp trait for generic exponentiation #1203

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

Conversation

bobzhang
Copy link
Contributor

@bobzhang bobzhang commented Nov 8, 2024

No description provided.

Copy link

From the provided git diff output, several observations and suggestions can be made regarding the changes in the MoonBit code:

  1. Trait Definition and Implementation:

    • Problem: The addition of the Exp trait and its implementations for Double and Int is a significant change. However, the implementation for Double seems to call a method exp() on inp, which is not defined in the standard MoonBit context provided.
    • Suggestion: Ensure that the exp() method for Double is either a built-in method or explicitly defined elsewhere in the codebase. If it's a custom method, it should be defined to avoid compilation errors.
  2. Generic Function for exp:

    • Problem: The new exp function is now generic and constrained by the Exp trait. This is a good practice for polymorphism but requires that all types T passed to exp must implement the Exp trait.
    • Suggestion: Verify that all intended types that will be used with this function indeed implement the Exp trait. This includes not only Double and Int but any other types that might be used in future or external implementations of the trait.
  3. Trait Implementation for Int:

    • Problem: The implementation of the Exp trait for Int converts the integer to a double using to_double() and then calls exp(). This conversion might introduce precision issues or inefficiencies if not handled properly.
    • Suggestion: Ensure that the conversion from Int to Double and subsequent exp() call meets the performance and precision requirements for your application. If precision is critical, consider alternative approaches or documenting the potential implications of this conversion.

These observations focus on potential issues related to method definitions, type constraints, and data type conversions that could affect the correctness or performance of the code.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 3813

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 80.196%

Files with Coverage Reduction New Missed Lines %
/Users/runner/work/core/core/math/exp.mbt 1 66.67%
Totals Coverage Status
Change from base Build 3812: -0.01%
Covered Lines: 4345
Relevant Lines: 5418

💛 - Coveralls

pub fn exp(input : Double) -> Double {
trait Exp {
exp(Self) -> Double
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's the expected return type of exp function?
should it be Self, Double or Float?

Copy link
Contributor

@hackwaly hackwaly Nov 8, 2024

Choose a reason for hiding this comment

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

I think Self is a proper choose.

If (1 : Int).exp(x) need a Double result, users always can turn it to Double first.

1.to_double().exp(x)

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