AMM Add Number class and associated algorithms #4145

HowardHinnant · 2022-04-13T21:10:16Z

High Level Overview of Change

Initial check-in for general purpose decimal floating point Number type with power raised to rational exponent algorithm.

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

* Conversions to Number are implicit * Conversions away from Number are explicit and potentially lossy * If lossy, round to nearest, and to even on tie

* Return 0 if abs(x) < limit, else returns x

gregtatcam

LGTM. I reviewed/tested only from integration into AMM point of view.

scottschurr

I'm not a good enough numerics person to have an opinion on the algorithms here. So I have to rely on tests returning results that I'd expect. There are some places where the test coverage is not very good, so I'd very much like to see some additional coverage. That will give me better confidence with the algorithms. Finding the cube root of -27 might be interesting, for example. And maybe exercising some of the places that you expect errors to be thrown.

Beyond that I had a few constexpr, naming, and structural thoughts for you to consider. But they are at your discretion. Let me know what you think. Thanks.

scottschurr · 2022-05-03T17:45:34Z

src/ripple/basics/Number.h

+{
+    using rep = std::int64_t;
+    rep mantissa_{0};
+    int exponent_{-2'147'483'648};


I might be inclined to use std::int32_t rather than int. That will increase behavioral similarities across platforms.

I'd also be inclined to use std::numeric_limits<std::int32_t>::lowest() in place of -2'147'483'648.

I attempted to minimize differences between Number and IOUAmount. The latter uses int here. However I've adopted your suggestion about numeric_limits.

scottschurr · 2022-05-03T20:14:16Z

src/ripple/basics/Number.h

+        explicit unchecked() = default;
+    };
+
+    explicit Number() = default;


I think the compiler treats this constructor as constexpr since both members are initialized, so it might be nice to mark it constexpr explicitly.

scottschurr · 2022-05-03T20:52:33Z

src/ripple/basics/Number.h

+    Number(rep mantissa);
+    explicit Number(rep mantissa, int exponent);


I think these two constructors could be declared constexpr if we made normalize() constexpr and moved the normalize() implementation into the header file. I don't see anything in the normalize() implementation that would prevent if from being used during constant evaluation. But maybe I'm missing something?

I'd rather not move normalize() into the header because of its size. The compiler probably won't inline it, which is good, but then the linker has to spend time eliminating duplicate outlined copies, which is bad. My policy is to not risk the code bloat or the extra linker work unless there is a benefit that outweighs these costs.

However sometimes I do want to have constexpr values. Thus I introduced:

explicit constexpr Number(rep mantissa, int exponent, unchecked) noexcept;

and

constexpr bool isnormal() const noexcept;

Here is an example use: 9968d47#diff-6c9395b2018418d46bb1c2584aa296d79ab620de7615538c6734648ca6eb71aeR357-R362

scottschurr · 2022-05-03T21:47:37Z

src/test/basics/Number_test.cpp

+    test_to_integer()
+    {
+        testcase("test_to_integer");
+        Number x[]{


For this test case in particular, having the inputs and output split into separate arrays makes the test hard to read and maintain. What would you think of making a struct that has both the input and the expected result and making an array of that struct? Maybe something like this:

struct OneTest { Number num; std::int64_t i; }; // clang-format off static constexpr OneTest tests[]{ // Number int {Number{ 0 }, 0}, // ... {Number{-9'999'999'999'999'999, 2}, -999'999'999'999'999'900}, // ... }; // clang-format on for (auto const& test : tests) { auto j = static_cast<std::int64_t>(test.num); BEAST_EXPECT(j == test.i); }

You could consider a similar approach to the earlier tests as well, but I think it's a real win here.

Thanks, I've adopted something similar to what you suggest.

scottschurr · 2022-05-03T23:55:42Z

src/ripple/basics/Number.h

+    operator<(Number const& x, Number const& y) noexcept
+    {
+        // If the two amounts have different signs (zero is treated as positive)
+        // then the comparison is true iff the left is negative.


I'm lovin' the comments in this section! Thanks!

scottschurr · 2022-05-05T00:10:22Z

src/ripple/basics/impl/Number.cpp

+        return one;
+    if (n == 1)
+        return f;
+    auto r = power(f, n / 2);


This power() function is untested from here down. This seems like the interesting stuff? Perhaps some tests could be added to exercise this part?

scottschurr · 2022-05-05T00:13:31Z

src/ripple/basics/Number.h

+    friend constexpr bool
+    operator<=(Number const& x, Number const& y) noexcept
+    {
+        return !(y < x);
+    }
+
+    friend constexpr bool
+    operator>=(Number const& x, Number const& y) noexcept
+    {
+        return !(x < y);
+    }


These operators are untested. Would probably be good to at least add a couple touch tests.

Added tests.

scottschurr · 2022-05-05T00:17:12Z

src/ripple/basics/Number.h

+inline Number&
+Number::operator++()
+{
+    *this += Number{1000000000000000, -15, unchecked{}};
+    return *this;
+}
+
+inline Number
+Number::operator++(int)
+{
+    auto x = *this;
+    ++(*this);
+    return x;
+}
+
+inline Number&
+Number::operator--()
+{
+    *this -= Number{1000000000000000, -15, unchecked{}};
+    return *this;
+}
+
+inline Number
+Number::operator--(int)
+{
+    auto x = *this;
+    --(*this);
+    return x;
+}


These operators are untested.

Added tests.

scottschurr · 2022-05-05T00:19:21Z

src/ripple/basics/impl/Number.cpp

+    {
+        if (exponent_ >= maxExponent)
+            throw std::overflow_error("Number::normalize 1");
+        m /= 10;
+        ++exponent_;
+    }


The body of this while loop is untested.

Added tests.

scottschurr · 2022-05-05T00:20:26Z

src/ripple/basics/impl/Number.cpp

+    {
+        *this = Number{};
+        return;
+    }


The body of this if is untested.

Added tests.

scottschurr · 2022-05-16T23:19:05Z

src/ripple/basics/impl/Number.cpp

+
+// The Guard class is used to tempoarily add extra digits of
+// preicision to an operation.  This enables the final result
+// to be correctly rounded to the internal precision of Number.


Thanks for this comment. It really helps me understand what Guard is doing.

scottschurr · 2022-05-17T01:24:32Z

src/test/basics/Number_test.cpp

+            {Number{-1}, 0, Number{1}},
+            {Number{5, -1}, 0, Number{0}},


I think these calls are asking for the zeroth root of -1 and 0.5 respectively? If that reading is correct, a couple of non-authoritative) web sites I checked suggest that the zeroth root of any number (possibly excepting 1) is undefined:

https://math.stackexchange.com/questions/962807/does-the-zeroth-root-exist

https://www.quora.com/What-is-the-zeroth-root-of-any-number

So I'm wondering if the better answer for taking the zeroth root of a number is to throw? I'm open to being wrong on this call...

I went with Annex F of the C standard: https://cigix.me/c17#F.10.4.4.p1

pow(−1, ±∞) returns 1.
pow(x, +∞) returns +0 for |x| < 1.

Annex F is consistent with the IEEE floating point standards: https://cigix.me/c17#F.1.p1

I did take the liberty of interpreting 1/0 as +∞.

I'm great with that call. How would you feel about adding comments to the root(Number, unsigned) and power(Number, unsigned, unsigned) declarations in Number.h that the implementations are consistent with Annex F of the C standard and with IEEE floating point standards?

scottschurr · 2022-05-17T01:38:09Z

src/test/basics/Number_test.cpp

+            {Number{-1}, 1, 0, Number{1}},
+            {Number{-1, -1}, 1, 0, Number{0}},


Given a little bit of research, I'm thinking that having a 0 in the d parameter of root should be an undefined result and so should throw. Am I misunderstanding?

I agree that there is room for debate here. But I thought the safest thing to do was follow the established C and IEEE standards.

scottschurr

First of all, the unit test coverage is outstanding! Thanks for putting in the effort there.

There are a couple of small changes I'd like to see:

Rename clip() to something like noiseGate().
I'd like to see comments added to root(Number, unsigned) and power(Number, unsigned, unsigned) that they comply with Annex F of the C standard.
See whether it makes sense to pass Number const& to the two power functions.

Assuming those are addressed this looks like it's good to go as far as I can see. Nice work!

scottschurr · 2022-05-17T17:09:45Z

src/ripple/basics/Number.h

+// Returns f^(n/d)
+
+Number
+power(Number f, unsigned n, unsigned d);


Most of the functions in this header pass Number const&. That could be done here too.

scottschurr · 2022-05-17T17:11:29Z

src/ripple/basics/Number.h

+// to find the root of the polynomial g(x) = x^d - f
+
+Number
+root(Number f, unsigned d);


The implementation of root modifies the passed in f, so there's a good reason to be passing a non-ref Number here.

Before we add these functions, I'd like to make sure they are needed. We need to be able to take the square root of a number, and we need to be able to square a number (multiply is likely good enough). I'd like to not add root and ``pow` until we know we need them for sure.

AMM equations require at a minimum root in case of equal weights. pow is required if unequal weights is supported.

@gregtatcam Don't we only need square root, not a general root function?

Correct, just the square root.

For the unequal weights case (if we support that), we need the general root function to support the power raise to a general rational function.

scottschurr · 2022-05-17T17:12:14Z

src/ripple/basics/Number.h

+// Uses a log_2(n) number of multiplications
+
+Number
+power(Number f, unsigned n);


Most of the functions in this header pass Number const&. That could be done here too.

See comment on root.

雷达啥时候回来

你们是不是雷达实验室的人

seelabs

I'm a little hesitant to add another numeric class to rippled. Do we really need to add a new one?

seelabs · 2022-05-17T18:32:30Z

src/ripple/basics/Number.h

+Number
+power(Number f, unsigned n, unsigned d);
+
+// Return 0 if abs(x) < limit, else returns x


I'd like to think of a better name. I agree that this shouldn't be named clip, but noiseGate doesn't really give an intuition for what this function does. I'll try to think of something too. Maybe squelch?

seelabs · 2022-05-17T18:34:48Z

src/ripple/basics/Number.h

+// to find the root of the polynomial g(x) = x^d - f
+
+Number
+root(Number f, unsigned d);


Before we add these functions, I'd like to make sure they are needed. We need to be able to take the square root of a number, and we need to be able to square a number (multiply is likely good enough). I'd like to not add root and ``pow` until we know we need them for sure.

seelabs · 2022-05-17T18:36:32Z

src/ripple/basics/Number.h

+// Uses a log_2(n) number of multiplications
+
+Number
+power(Number f, unsigned n);


See comment on root.

HowardHinnant · 2022-05-17T19:04:05Z

I've addressed all of @scottschurr's comments. On noiseGate, happy to change it again to whatever the consensus is.

gregtatcam · 2022-05-17T19:19:43Z

I'm a little hesitant to add another numeric class to rippled. Do we really need to add a new one?

AMM equations are not like other math ops in the rippled and are neither STAmount or XRPAmount or IOUAmount and can mix all of these types. Moreover, using multiply and divide makes these equations unreadable and error prone. The Number class makes AMM equations easy to implement and understand.

scottschurr · 2022-05-17T19:19:44Z

src/ripple/basics/impl/Number.cpp

+// This function, and power(Number f, unsigned n, unsigned d)
+// treat corner cases such as 0 roots as advised by Annex F of
+// the C standard, which itself is consistent with the IEEE
+// floating point standards.


haihai423 · 2022-05-23T09:09:26Z

radar的贡献者，雷达还有希望吗

zhangdongwei123 · 2022-05-23T09:15:43Z

你是谁？ | | ***@***.*** | | ***@***.*** |

…

---- 回复的原邮件 ---- | 发件人 | ***@***.***> | | 日期 | 2022年05月23日 17:09 | | 收件人 | ***@***.***> | | 抄送至 | ***@***.******@***.***> | | 主题 | Re: [ripple/rippled] AMM Add Number class and associated algorithms (PR #4145) | radar的贡献者，雷达还有希望吗 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

zhangdongwei123 · 2022-05-23T09:16:19Z

我也是玩家 | | ***@***.*** | | ***@***.*** |

…

---- 回复的原邮件 ---- | 发件人 | ***@***.***> | | 日期 | 2022年05月23日 17:09 | | 收件人 | ***@***.***> | | 抄送至 | ***@***.******@***.***> | | 主题 | Re: [ripple/rippled] AMM Add Number class and associated algorithms (PR #4145) | radar的贡献者，雷达还有希望吗 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

haihai423 · 2022-05-23T09:34:41Z

我也是玩家，你是玩雷达的吗，

scottschurr · 2022-06-23T21:01:29Z

@HowardHinnant, should this pull request be closed? It looks like you have a competing pull request: #4192. Your call.

HowardHinnant · 2022-06-23T23:57:41Z

Closed in lieu of #4192.

AMM Add Number class and associated algorithms

9968d47

HowardHinnant added the AMM label Apr 13, 2022

HowardHinnant requested a review from gregtatcam April 13, 2022 21:10

HowardHinnant assigned gregtatcam Apr 13, 2022

Add conversions between Number, XRPAmount and int64_t

cde8a32

* Conversions to Number are implicit * Conversions away from Number are explicit and potentially lossy * If lossy, round to nearest, and to even on tie

HowardHinnant force-pushed the Number branch from 8002038 to cde8a32 Compare April 15, 2022 20:54

HowardHinnant requested a review from scottschurr April 15, 2022 20:58

HowardHinnant assigned scottschurr Apr 15, 2022

HowardHinnant added 4 commits April 18, 2022 14:21

Add clip

2ad28b3

* Return 0 if abs(x) < limit, else returns x

Add implicit conversion from STAmount to Number

0008e14

[FOLD] Miscellaneous minor fixes

6dfd0cb

Add tests

1975e8b

gregtatcam approved these changes Apr 25, 2022

View reviewed changes

scottschurr reviewed May 5, 2022

View reviewed changes

[FOLD] Address reviewer comments

22370e3

HowardHinnant force-pushed the Number branch from 192b7e6 to 22370e3 Compare May 13, 2022 19:52

scottschurr reviewed May 16, 2022

View reviewed changes

scottschurr reviewed May 17, 2022

View reviewed changes

scottschurr approved these changes May 17, 2022

View reviewed changes

seelabs reviewed May 17, 2022

View reviewed changes

[FOLD] Address reviewer comments

5877ebb

scottschurr reviewed May 17, 2022

View reviewed changes

[FOLD] Rename noiseGate to squelch

bee18ca

[FOLD] Add tests for int_min and int_max

73fc418

HowardHinnant closed this Jun 23, 2022

		Number(rep mantissa);
		explicit Number(rep mantissa, int exponent);

		{Number{-1}, 1, 0, Number{1}},
		{Number{-1, -1}, 1, 0, Number{0}},

AMM Add Number class and associated algorithms #4145

AMM Add Number class and associated algorithms #4145

Conversation

HowardHinnant commented Apr 13, 2022

High Level Overview of Change

Context of Change

Type of Change

gregtatcam left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardHinnant May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardHinnant commented May 17, 2022

gregtatcam commented May 17, 2022

Choose a reason for hiding this comment

haihai423 commented May 23, 2022

zhangdongwei123 commented May 23, 2022 via email

zhangdongwei123 commented May 23, 2022 via email

haihai423 commented May 23, 2022

scottschurr commented Jun 23, 2022

HowardHinnant commented Jun 23, 2022

HowardHinnant May 17, 2022 •

edited

Loading