Mistralys / subsetsum

PHP SubsetSum implementation
MIT License
1 stars 1 forks source link

PHP min version #1

Closed rafark closed 3 years ago

rafark commented 3 years ago

Thank you for writing this! It works like an absolute charm.

Just as an FYI, it requires PHP 7.1 (which isn't stated in the docs or in packagist). And for everyone interested, you can make it PHP 7.0 compatible by removing the nullable return types (just 2 instances: ?array).

PS. I'm your first star.

Mistralys commented 3 years ago

Hi rafark, glad to hear you could use it!

I added the PHP 7.1 dependency by removing the nullable array return types. I don't want to go as far as PHP 7.0 though, because that would mean removing all the return types.

https://github.com/Mistralys/subsetsum/releases/tag/1.0.1

Cheers, and thanks for the star :)

rafark commented 3 years ago

Found a bug.

The set: 22.4, 16.2, 10, 6

and a sum of :

44.6

return nothing.

After debugging with var_dump(), in SubsetSum.php:338, both $s and $search are 44.6 (float), but $s === $search returns false.

// we have found a match! if($s === $search) SubsetSum.php:338

After some research, I found that comparing two floats with the comparison operator (===) is problematic. I've read that floating point handling in PHP can be unstable, but this is the first time in years I've personally seen something like this.

Here are several solutions (both with pros and cons) if you're still maintaining this project:

https://stackoverflow.com/questions/3148937/compare-floats-in-php

And here are some nice reads on the topic of floating points in programming languages:

https://stackoverflow.com/questions/3726721/php-floating-number-precision

https://stackoverflow.com/questions/588004/is-floating-point-math-broken

rafark commented 3 years ago

If you're curious, I went with this approach:

// we have found a match! if(bccomp((string) $s, (string) $search, $this->precision) === 0)

Mistralys commented 3 years ago

Thanks for the bug report, @rafark! I also saw what I suspect was your comment on stackoverflow. I created an issue for it (#2 ), and will have a look :)