[ARITH] Fix intersect of modular set #2726
Conversation
merrymercy
force-pushed
the
modular-set
branch
2 times, most recently
from
592ac61 to
c064d75
Compare
merrymercy
added
status: need review
and removed
status: need update
| @@ -32,10 +32,27 @@ TVM_STATIC_IR_FUNCTOR(IRPrinter, vtable) | |||
|
|
|||
|
|
|||
| // internal entry for const int bound | |||
| // This condition holds for all instances: coeff >= 0, base in [0, coeff] | |||
There was a problem hiding this comment.
base in [0, coeff) for coeff != 0, for coeff=0 base can be any number that indicates constant
| Entry(int64_t coeff, int64_t base) { | ||
| this->coeff = coeff; | ||
|
|
||
| if (coeff < 0) { |
There was a problem hiding this comment.
I am not sure if we want to support such smart conversion here, coeff<0 could be more likely an error, and we can do CHECK_GE(coeff, 0)
| coeff = -coeff; | ||
| } | ||
|
|
||
| if (coeff != 0) { |
There was a problem hiding this comment.
because this constructor is "too smart", I would recommend us to create a static function instead. C++ constructor also have a restriction of not throw exception, having a factory function and put checks there avoids the problem
Entry::make(coeff, base);
There was a problem hiding this comment.
C++ constructor also have a restriction of not throw exception
As far as I know, throwing exceptions from constructors is absolutely OK. (Unless you want to avoid exceptions completely and return an error code or an optional, in which case you have to use a factory function.)
| * \return The GCD of a and b. | ||
| */ | ||
| static int64_t ExtendedEuclidean(int64_t a, int64_t b, int64_t *x, int64_t *y) { | ||
| int64_t s = 0, old_s = 1; |
There was a problem hiding this comment.
add invariance check CHECK_GE(a, 0);
There was a problem hiding this comment.
Making the algorithm work for negative arguments would be a better way in my opinion. GCD and Bezout coefficients are defined even for negative numbers, and requiring the arguments to be non-negative will make this function harder to use for some applications.
| * \param y The solution of y. | ||
| * \return The GCD of a and b. | ||
| */ | ||
| static int64_t ExtendedEuclidean(int64_t a, int64_t b, int64_t *x, int64_t *y) { |
| */ | ||
| static int64_t ExtendedEuclidean(int64_t a, int64_t b, int64_t *x, int64_t *y) { | ||
| int64_t s = 0, old_s = 1; | ||
| int64_t r = b, old_r = a; |
There was a problem hiding this comment.
Think a bit more about how to make the code more readable: comment a bit on what is r and what is s, so it is easier for others to understand
There was a problem hiding this comment.
Some useful comment that contains the derivation
// Extended Euclidean algorithm
// initial condition:
// a * 0 + b * 1 = a
// a * 1 + b * 0 = b
//
// Iteration (r2 < r1):
// a * x1 + b * y1 = r1
// a * x2 + b * y2 = r2
// The above two eqs can derive the following eq (q = r2 / r1)
// a * (x1 - x2 * q) + b * (y1 - y2 * q) = r2 - r1 * q = r3
// Because r3 < r2, the iteration can eventually terminate
| return base; | ||
| static Entry Intersect(Entry x, Entry y) { | ||
| int64_t n, m; | ||
| int64_t a = x.coeff, b = x.base, c = y.coeff, d = y.base; |
There was a problem hiding this comment.
A bit more comment on the derivation
There was a problem hiding this comment.
Let z be the integer, we know z satisfies
z = x * c1 + b1
z = y * c2 + b2
... derivation
z's general pattern
|
closed by #2904 |
fix todo in #2668
cc @tqchen