search

PaddiM8 / kalker

Scientific calculator with math syntax that supports user-defined variables and functions, complex numbers, and estimation of derivatives and integrals
https://kalker.xyz
MIT License
1.59k stars 70 forks source link

`gcd` shows wrong result when input number is too large #119

Open abiriadev opened 1 year ago

abiriadev commented 1 year ago

Actual Behavior

>> gcd(500000000000000000, 500000000000000002)
500 000 000 000 000 000 ≈ 5×10^17

Expected result

2

Suggestion

I think it would be more helpful to warn a user if the input value is larger than some amount. And I am willing to contribute or send PR if needed.

Regards.

PaddiM8 commented 1 year ago

Hm yeah. Not even sure why this happens. It would make sense to warn the user, but I'm not sure there's a way to do that currently, while still giving a result. If we figure out when exactly this happens, it might make sense to simply return an error if the input is too large (for now at least). It seems to be when both are a certain size, but I haven't found a more specific pattern yet.

abiriadev commented 1 year ago

I'd be happy to help or contribute in any way if I can (and have enough time).

Could you please point me to where the gcd implementation is located? I think it might be a good starting point to identify the issue.

PaddiM8 commented 1 year ago

Great! That might speed things up, since I also have limited time right now. The function is implemented here: https://github.com/PaddiM8/kalker/blob/9b55f894427b84b43065579bbf3900eb7d46daf6/kalk/src/prelude/mod.rs#L612

KalkValue represents a value in kalk, usually a number. Normally, this is a heap allocated big float, but for the web version it's just an f64, since the big float library doesn't support WebAssembly. To be able to write generic code despite this, there is a float! macro that turns a number into the relevant type.

dvishal485 commented 7 months ago

How can float! be useful here? I don't think it will resolve issue regarding number not being able to stored in heap with precision. Am I missing something here?

https://github.com/PaddiM8/kalker/blob/9b55f894427b84b43065579bbf3900eb7d46daf6/kalk/src/kalk_value/mod.rs#L23-L38

I would love to contribute if I understand the issue completely.

PaddiM8 commented 7 months ago

The purpose of float! is to create a rug::Float or f64, depending on if the rug feature is enabled. Unfortunately it doesn't get the configured precision.

dvishal485 commented 7 months ago

I believe it has something to do with the fact that float! macro always get an f64 value because parsing of Literals was done with f64.

https://github.com/PaddiM8/kalker/blob/e9c9e81d078d1adacde7168ed318fa44b612b7c9/kalk/src/ast.rs#L22

https://github.com/PaddiM8/kalker/blob/e9c9e81d078d1adacde7168ed318fa44b612b7c9/kalk/src/ast.rs#L13-L30

So no matter what you do with float!(), you will always be passing it f64 with less precision and hence value may be different than what it was asked to parse originally.

I don't know if I am right on this, but it is a best guess about the source of "error" I could make as a newbie.

PaddiM8 commented 7 months ago

Hm yeah that sounds about right yeah. It should probably be a rug float when the rug feature is enabled.

dvishal485 commented 7 months ago

I have changed a lot of things in codebase to fix this. Everything compiles and test out just fine with rug feature enabled as well as disabled. Making a PR for the same.

This would parse strings with correct precision instead of loading them into f64 and then making them Float. Tested with the input 500000000000000002 which works fine now. You may want to rearrange the codebase to accommodate some functions I added, and rename/remove some existing functions.

Unfortunately for without rug crate, it would use f64.