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

Add float.modulo #582

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Add float.modulo #582

merged 3 commits into from
Jul 17, 2024

Conversation

dalprahcd
Copy link
Contributor

@dalprahcd dalprahcd commented May 9, 2024

Should close #525 by adding the float.modulo function.

I also tried implementing the float.remainder function as:

pub fn modulo(dividend: Float, by divisor: Float) -> Result(Float, Nil) {
  case divisor {
    0.0 -> Error(Nil)
    _ -> Ok(dividend -. round(dividend /. divisor) *. divisor)
  }
}

But round returns an Int and I could not find an easy way to convert it back to Float. A little bit off-topic, but why does it return an Int it should return a Float right?

@lpil
Copy link
Member

lpil commented May 10, 2024

A little bit off-topic, but why does it return an Int it should return a Float right?

If round returned a float how would you round a float to an int?

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

/// ```gleam
/// modulo(-13.0, by: -3.0)
/// // -> Ok(-1.0)
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Can we have fewer examples please? It's a little overwhelming I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments were copied from the int.modulo function, they explain all the possible behaviours.

  • When divisor is non-zero and when it is zero.
  • That the modulo follows the sign of the divisor.

Can we keep the examples?

Copy link
Member

Choose a reason for hiding this comment

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

There is too much information here, please reduce it. The examples section is not suitable for an exhaustive description of the behaviour.

|> should.equal(Ok(-2.0))

float.modulo(-13.0, by: -3.0)
|> should.equal(Ok(-1.0))
Copy link
Member

Choose a reason for hiding this comment

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

All of these are whole numbers. Let's have tests for other numbers too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, could you help me solve this failing test?

Failures:

  1) gleam/float_test.modulo_test
     Values were not equal
     expected: Ok(1.3)
          got: Ok(1.3000000000000007)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check https://hexdocs.pm/gleam_stdlib/gleam/float.html#loosely_equals maybe

But then I believe I would need to import the result library to unwrap the value before, right? Is it ok adding this dependency?

@dalprahcd
Copy link
Contributor Author

If round returned a float how would you round a float to an int?

We could explicit convert float to int by implementing int.to_float and float.to_int (if not already implemented). Then we could also expand the float.round function to accept how many decimals places we want to round the float.

Finally, we would have 3 options to round a float to int: float.floor, float.ceiling, float.round(x, 0). And the user would be responsible to choose which one is more suitable.

Calling float.to_int would essentially cut out the decimal places.

@NicklasXYZ
Copy link
Contributor

If round returned a float how would you round a float to an int?

We could explicit convert float to int by implementing int.to_float and float.to_int (if not already implemented). Then we could also expand the float.round function to accept how many decimals places we want to round the float.

Finally, we would have 3 options to round a float to int: float.floor, float.ceiling, float.round(x, 0). And the user would be responsible to choose which one is more suitable.

Calling float.to_int would essentially cut out the decimal places.

This is how I've implemented it in the the community math lib :)
I guess the question is also related to this closed issue: #625 (comment)

@dalprahcd
Copy link
Contributor Author

Hello @lpil and @inoas, sorry for being away for so long.

I wish to merge this PR, what are the remaining steps?

@lpil
Copy link
Member

lpil commented Jul 10, 2024

The comments from my review haven't been addressed yet!

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

@lpil lpil merged commit 2117242 into gleam-lang:main Jul 17, 2024
7 checks passed
@dalprahcd dalprahcd deleted the feature-add-float-modulo-and-remainder branch July 21, 2024 07:33
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.

floating point modulo
4 participants