Skip to content

feat: LessThan operator implementation #13

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raizo07
Copy link

@raizo07 raizo07 commented Apr 18, 2025

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

LessThan operator implementation for fixed point

Does this introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

@raphaelDkhn raphaelDkhn left a comment

Choose a reason for hiding this comment

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

@raizo07 Do the tests pass on your side? The tests fail on GH and on my side. There's a problem with the constraints

/// - result is 1 if self < other, 0 otherwise
/// - remainder is always 0 (to maintain consistency with other operations that return remainders)
#[inline]
pub fn lt(&self, other: &Self) -> (Self, Self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The remainder is unnecessary here. We don't need to return a tuple.

#[inline]
pub fn lt(&self, other: &Self) -> (Self, Self) {
if self.0 < other.0 {
(Self(1), Self(0)) // True case: self < other
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should return the fixed-point representation of 1, instead of 1 directly. Otherwise, the result is unscaled.

@raizo07
Copy link
Author

raizo07 commented Apr 23, 2025

@raphaelDkhn will implement the necessary changes and re-run the test again

@raizo07
Copy link
Author

raizo07 commented Apr 26, 2025

@raizo07 Do the tests pass on your side? The tests fail on GH and on my side. There's a problem with the constraints

@raphaelDkhn I'm currently getting this error for the second test

image

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.

2 participants