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

Use a math library other than moontoast/math #109

Closed
ramsey opened this issue Mar 26, 2016 · 14 comments
Closed

Use a math library other than moontoast/math #109

ramsey opened this issue Mar 26, 2016 · 14 comments
Labels
Milestone

Comments

@ramsey
Copy link
Owner

@ramsey ramsey commented Mar 26, 2016

A math library is not required for this library unless users want to operate on the UUID as a 128-bit integer. For this purpose, the library has suggested moontoast/math as an optional dependency, and internally, this is what ramsey/uuid uses, if it is present. However, moontoast/math is no longer maintained.

I will either create a ramsey/math library that allows the use of different adapters for working with large numbers, or I want to find an existing math library that provides similar functionality.

@ramsey ramsey added this to the Version 4.0.0 milestone Mar 26, 2016
@ramsey
Copy link
Owner Author

@ramsey ramsey commented Mar 26, 2016

Here's an initial, stubbed-out approach to a ramsey/math library: https://github.com/ramsey/math

Again, I'm not necessarily of the mindset that the math library must be my own. These are just some thoughts I had on how a math library should work.

@oleg-andreyev
Copy link

@oleg-andreyev oleg-andreyev commented Sep 11, 2018

@ramsey maybe it's worth using brick/math (or any other library) instead of maintaining own version?

Also I think it would be better to have an ability to set custom math library via FeatureSet and provide "default" BigNumberConverter as it is now.

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Sep 11, 2018

I'm still giving this a lot of thought. Ideally, ramsey/uuid wouldn't need to know about any external number library at all. I would love to return an unsigned 128-bit integer (or at least 2 unsigned 64-bit integers) and let the caller handle the number by passing it to their preferred library.

To do this, I would need to return the number in string format, since PHP can't handle unsigned 64-bit integers, much less unsigned 128-bit integers. I can do this using bcmath or gmp, but that requires a system dependency, and not everyone has those installed. Likewise, I could use moontoast/math to create this string and return it (as just a string), but again, moontoast/math depends on bcmath.

If there were a pure PHP polyfill for bcmath or gmp, then I could use that to perform the calculations and return a string number. Then, the caller can use whatever math library they wish. This is my preferred approach, but I have yet to find a pure PHP polyfill for bcmath or gmp. phpseclib does provide a pure PHP implementation, but it is far more than I need, and I prefer that the polyfill provide the bcmath or gmp functions rather than an abstraction around them.

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Jan 7, 2019

After looking closer at brick/math, I think it's the solution I want. They use gmp or bcmath, if available, falling back to their own pure PHP big number calculator, if not.

However, I don't want to return brick/math objects, since I don't want external code to depend on it. So, I'll use brick/math internally and return string representations of integers.

Another option, for better type-safety, is to provide a Ramsey\Uuid\Integer value object that is returned for anything that returns an integer or Moontoast\Math object today.

@nicholasnet
Copy link

@nicholasnet nicholasnet commented Mar 13, 2019

I stumbled into this issue by co-incidence since I found that moontoast has not received any update recently. Anyway that is not a big issue.

@ramsey How about creating interface for Integer and wrap brick/math to return that interface. In that way we do not have to re-write brick/math and we will get type-safety as well. If you guys are ok with it. I can create PR for it.

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Mar 13, 2019

@nicholasnet That sounds great to me. Go for it!

@nicholasnet
Copy link

@nicholasnet nicholasnet commented Mar 14, 2019

I did some investigation it seems like all we need is these methods in interface for our use case.

  • multiply
  • add
  • substract
  • round
  • parse
  • toBase (for stuff like convertFromBase10, covertToBase10 and such)

However, there some significant difference between Moontoast and Brick implementation. Most are minor like immutable and mutable stuff. But major ones are following

  • Brick further categories BigNumber to BigInteger, BigDecimal, BigRational
  • In Brick rounding needs to set explicitly in Moontoast it is chosen automatically.

So, my question is which implementation we need to choose I am guessing BigDecimal but we need to confirm.

Also considering this piece of code that we have currently in Converter/Time/BigNumberTimeConverter.php.

$ts = new BigNumber($timestamp, 20);
$ts->subtract('122192928000000000');
$ts->divide('10000000.0');
$ts->round();

return $ts->getValue();

What will be the Brick equivalent of it. Any Brick expert? I did this but tests are failing... for UuidTest::testGetDateTime32BitMoontoast :(

$ts = BigDecimal::ofUnscaledValue($timestamp, 20);
$ts = $ts->minus('122192928000000000');
$ts = $ts->dividedBy('10000000.0', 20, RoundingMode::HALF_DOWN);
$ts = $ts->dividedBy(1, 0, RoundingMode::HALF_DOWN);
$ts = $ts->toBigInteger();

return $ts->__toString();

We will need to nail down this wrinkle first because no matter which Math library we choose we need to make sure that existing implementation does not break and it is correct.

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Mar 14, 2019

I don't think the Integer needs that level of detail on it. It should probably just be a value object and contain the integer on it as a string value. Internally, ramsey/uuid would use Brick, but it wouldn't expose any of the math functionality to the outside. The Integer interface is just for type-safety, and once it's returned from the UUID library, it's up to the user to handle it with whatever library they wish.

As for how to handle the rounding with Brick, I can't answer that right now. I'll try to do some investigation to see how it works.

@LorenzoSapora
Copy link

@LorenzoSapora LorenzoSapora commented Jan 6, 2020

moontoast/math has officially been listed as abandoned by the author. Time to revisit this issue?

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Jan 6, 2020

I am the author of moontoast/math. 😄

I'm currently working on a replacement solution for the 4.x branch.

@LorenzoSapora
Copy link

@LorenzoSapora LorenzoSapora commented Jan 6, 2020

D'oh! Apologies

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Jan 6, 2020

No worries!

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Jan 12, 2020

@nicholasnet It turns out the proper rounding should have been "down" instead of "half down." The rounding method I'm using in 3.x has a bug in it; it always rounds to the nearest whole number, which is problematic when dealing with time, since the microseconds can end up rounding up to the next second, which can cause problems with the DateTime objects (i.e., if the microseconds are 500000 or more, it could round up to the next second and cause the hour, day, month, and year to advance to the next one).

I'm using brick/math in the 4.x branch, and here's the relevant code for the rounding with the time:

public function convertTime(string $timestamp): string
{
$timestamp = new IntegerValue($timestamp);
$unixTimestamp = $this->calculator->subtract(
$timestamp,
new IntegerValue('122192928000000000')
);
// Round down so that the microseconds do not shift the timestamp
// into the next second, giving us the wrong Unix timestamp.
$unixTimestamp = $this->calculator->divide(
RoundingMode::DOWN,
$unixTimestamp,
new IntegerValue('10000000')
);
return $unixTimestamp->toString();
}

This will change a little before I release it, since I want to support microseconds in the DateTime objects returned (see #90 and #152).

@ramsey
Copy link
Owner Author

@ramsey ramsey commented Jan 12, 2020

Closing, since this has been addressed in the 4.x branch.

@ramsey ramsey closed this Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants