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 gcd/lcm functions #73

Merged
merged 7 commits into from Oct 8, 2021
Merged

Add gcd/lcm functions #73

merged 7 commits into from Oct 8, 2021

Conversation

kiedtl
Copy link
Contributor

@kiedtl kiedtl commented Sep 30, 2021

This PR adds the gcd and lcm functions.

>> gcd(12, 18)
6
>> lcm(12, 18)
36

GCD is calculated with the modified Euclidean algorithm, and LCM is
calculated with:

             ⎛           ⎞
             ⎜    ⎜a⎜     ⎟
lcm(a, b) =  ⎜ ───────── ⎟ × ⎜b⎜
             ⎜ gcd(a, b) ⎟
             ⎝           ⎠

@kiedtl
Copy link
Contributor Author

kiedtl commented Sep 30, 2021

Hmm, what's the proper way to throw a non-terminating error from a
function?

@PaddiM8
Copy link
Owner

PaddiM8 commented Sep 30, 2021

This is a great addition!

Hmm, what's the proper way to throw a non-terminating error from a
function?

Hmm, calculator functions (like the ones you made) currently don't have a lot of options for error handling. It is assumed that any function will return a value. However, I believe some functions simply return NaN when a calculation isn't possible. You could do something like KalkNum::from(f64::NAN), I believe.

Now that I think about it, being able to throw error messages from inside these functions should probably be possible in the future. Then errors would be thrown by returning eg. CalcError::ErrorType(info) like in the rest of the code.

@kiedtl
Copy link
Contributor Author

kiedtl commented Sep 30, 2021

Now that I think about it, being able to throw error messages from inside these functions should probably be possible in the future. Then errors would be thrown by returning eg. CalcError::ErrorType(info) like in the rest of the code.

Definitely, it's better UX to throw an informative error message instead of silently returning NaN, which could be annoying to debug if the gcd/lcm call was part of a larger calculation/function call. However, I'll use the NaN solution for now :)

@kiedtl
Copy link
Contributor Author

kiedtl commented Oct 4, 2021

Alright, this is ready for review.

@kiedtl kiedtl closed this Oct 4, 2021
@kiedtl kiedtl reopened this Oct 4, 2021
@kiedtl
Copy link
Contributor Author

kiedtl commented Oct 4, 2021

Oops, wrong button

@kiedtl kiedtl marked this pull request as ready for review October 4, 2021 04:11
Copy link
Owner

@PaddiM8 PaddiM8 left a comment

Choose a reason for hiding this comment

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

Great! Would be good if you could also run rustfmt on the file (this should've been mentioned in the README, I'll add that now). Also, I noticed that, when you give it complex numbers, it differs a bit from what eg. WolframAlpha and MathWorks are saying. It gives a result like 10 - 5i while WolframAlpha says 5 + 10i. Would the latter maybe be more of an expected result by users?

@kiedtl
Copy link
Contributor Author

kiedtl commented Oct 5, 2021 via email

@PaddiM8
Copy link
Owner

PaddiM8 commented Oct 5, 2021

I'm assuming that both of those answers are correct... but I don't know
which one would be preferred.

Yeah this is what I'm thinking as well. When comparing to WolframAlpha, it seems to consistently give the same numbers, but often with the real and imaginary ones swapped, and the sign inverted. From what I can tell, it seems to give correct answers though, so it's probably good enough for now at least!

Copy link
Owner

@PaddiM8 PaddiM8 left a comment

Choose a reason for hiding this comment

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

The "0" constants need to be specified as f64 for it to work when not using rug. Eg. x.value.clone().fract() != 0f64. You can test this by running something like cargo test --no-default-features I believe.

@kiedtl
Copy link
Contributor Author

kiedtl commented Oct 8, 2021

Alright, working now

@PaddiM8
Copy link
Owner

PaddiM8 commented Oct 8, 2021

Nice! This looks good now, I'll go ahead and merge it. Excited to have this in kalker.

@PaddiM8 PaddiM8 merged commit e836802 into PaddiM8:master Oct 8, 2021
@PaddiM8
Copy link
Owner

PaddiM8 commented Jan 1, 2022

These can now be used on https://kalker.xyz!

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.

None yet

2 participants