Karatsuba added, 2 bugs are fixed #328

zhelenskiy · 2021-05-10T22:53:21Z

Karatsuba was added and increased benchmarks results.
The bug that was caused by abs(Int.MIN_VALUE) == Int.MIN_VALUE was fixed.
The equals function now behaves with instances of other types as all other objects do.

Incorrect equals fix test Incorrect overflow handling support

zhelenskiy · 2021-05-10T22:54:51Z

(Slack discussion)

breandan · 2021-05-10T23:45:28Z

I recently learned that E.A. Karatsuba has a fast method for evaluating transcendentals, which can be useful for working with certain probability distributions. It might be worth looking into at some point: https://en.wikipedia.org/wiki/FEE_method

zhelenskiy · 2021-05-11T00:48:19Z

@breandan However, the Karatsuba's algorithm that is subject of the PR is fast multiplication.

altavir

The changes in the core are not appropriate at the moment, they should be discussed in a separate issue first. Otherwise looks fine. It would be nice to have your benchmarking results in docs and a separate example in examples.

altavir · 2021-05-11T05:56:09Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/Algebra.kt

+        else -> powWithoutOptimization(exponent)
+    }
+
+    private fun T.powWithoutOptimization(exponent: ULong): T = when (exponent) {


The implementation details should not be present In generic algebra. Probably better to use extensions instead. Like Ring<T>.pow(T, exponent). This change needs additional discussion.

I completely agree with @altavir.

Actually, it should be placed there (as an extension). Thank you! I'll fix it.
Furthermore, it may be better to create a function in BigInt that would make it possible to use power for bigints without BigIntField { }. What do you think, @altavir @CommanderTvis? After inventing multiple receivers that should be replaced though.

altavir · 2021-05-11T05:56:35Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/Algebra.kt

@@ -250,6 +250,22 @@ public interface Ring<T> : Group<T>, RingOperations<T> {
     * neutral operation for multiplication
     */
    public val one: T
+
+    public fun T.pow(exponent: ULong): T = when {


Are there any real use-cases for Long power?

Do you mean negative exponent usage? If so, no. It is impossible to implement.

Or do you mean big range? If so, yes for some non-bigint rings. As we don't know what the rings are, we cannot say anything about the usage of exponents of UInt.MAX_VALUE.toULong().inc()..ULog.MAX_VALUE

CommanderTvis · 2021-05-11T09:03:42Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/Algebra.kt

+        else -> powWithoutOptimization(exponent)
+    }
+
+    private fun T.powWithoutOptimization(exponent: ULong): T = when (exponent) {


I completely agree with @altavir.

pklimai · 2021-05-11T17:11:32Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/BigInt.kt

+
+            val z0 = x0 * y0
+            val z2 = x1 * y1
+            val z1 = (x0 + x1) * (y1 + y0) - z0 - z2


Will it be more optimal to use the other formula for z1, namely $z1 = (x0-x1)*(y1-y0)+z2+z0$ ?

I changed it.

zhelenskiy · 2021-05-12T02:02:08Z

I have submitted what you suggested. Also, I fixed bugs, listed in Slack. I decreased the overhead of parsing of BigInt.

zhelenskiy · 2021-05-12T08:46:55Z

The changes in the core are not appropriate at the moment, they should be discussed in a separate issue first. Otherwise looks fine. It would be nice to have your benchmarking results in docs and a separate example in examples.

I think they should be discussed in Slack because it is already in the PR
Which results? Furthermore, there is still a Fourier -based multiplication method to be implemented first to publish results then.

zhelenskiy · 2021-05-12T11:17:38Z

I found that the addition of large integers is slower with KMath BigInt than with JVM BigInteger: I added benchmarks for that.

…igIntField usage removed

altavir · 2021-05-13T16:51:00Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/Algebra.kt

     */
    public val one: T
 }

+@UnstableKMathAPI
+public fun <T> Ring<T>.pow(base: T, exponent: ULong): T = when {


altavir · 2021-05-13T16:51:09Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/Algebra.kt

+}
+
+@UnstableKMathAPI
+public fun <T> Ring<T>.pow(base: T, exponent: UInt): T = pow(base, exponent.toULong())


altavir · 2021-05-13T16:55:57Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/Algebra.kt

+@UnstableKMathAPI
+public fun <T> Ring<T>.pow(base: T, exponent: UInt): T = pow(base, exponent.toULong())
+
+private fun <T> Ring<T>.powWithoutOptimization(base: T, exponent: ULong): T = when (exponent) {


Move pow operation to algebraExtensions.kt. It actually already have those functions: https://github.com/mipt-npm/kmath/blob/ca02f5406d06d18d07909f3d48508e1a99701fa5/kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/algebraExtensions.kt#L107, so you shoud probably just replace the implementation and use UInt instead of Int.

Why not ULong? Because of being consistent with Field<T>.power which has a stable API?

Also, I see a bug with Field.power:

public fun <T> Field<T>.power(arg: T, power: Int): T { require(power != 0 || arg != zero) { "The $zero raised to $power is not defined." } if (power == 0) return one if (power < 0) return one / (this as Ring<T>).power(arg, -power) return (this as Ring<T>).power(arg, power) }

If [power] is Int.MIN_VALUE, this will fail

The implementation could be wrong in corner cases. It is not properly tested. As for ULong, the major bulk of operations on JVM and in kotlin use Int. I can't see a case where people would use more than Int for a power (please show them if you know them) and using Ulong would force users to do an additional unnecessary conversion.

I am absolutely sure that there is no need to power numbers by ULong ranged numbers, however, this may be ok for some other Rings (albeit I don't know such ones).

Also, 0^0 is not defined here while it is 1 in java's BigInt. Which convention to choose?

public fun <T> Ring<T>.power(arg: T, power: Int): T { require(power >= 0) { "The power can't be negative." } require(power != 0 || arg != zero) { "The $zero raised to $power is not defined." } if (power == 0) return one var res = arg repeat(power - 1) { res *= arg } return res }

Let's stick to UInt for now because I still can't imagine a case for longs. Since we are using extensions, it is easy to add another one if somebody needs it. As for 0^0, please fix it. I don't remember where this method comes from, but it was quite obviously forgotten.

Do you mean that 0^0 is expected to be 1?

I published the changes.

altavir · 2021-05-13T16:57:54Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/BigInt.kt

-    else
-        -this * kotlin.math.abs(other).toUInt()
+    @UnstableKMathAPI
+    public fun pow(other: ULong): BigInt = BigIntField { pow(this@BigInt, other) }


BigIntField.pow(this, other) should be more concise.

zhelenskiy

The was a mistake in the added code It was discussed in Slack. It was fixed in the further commits to the repo.

zhelenskiy · 2021-05-14T07:09:09Z

kmath-core/src/commonMain/kotlin/space/kscience/kmath/operations/algebraExtensions.kt

-public fun <T> Field<T>.power(arg: T, power: Int): T = when {
-    power < 0 -> one / (this as Ring<T>).power(arg, if (power == Int.MIN_VALUE) Int.MAX_VALUE.toUInt().inc() else (-power).toUInt())
-    else -> (this as Ring<T>).power(arg, power.toUInt())
+public fun <T> Field<T>.power(arg: T, power: UInt): T = when {


@altavir You are wrong here: Field can have negative power as Field has division. That was even in the previous implementation.

It is reverted.

zhelenskiy added 4 commits May 10, 2021 17:57

Fast power is added

1716b7f

Further optimization of power is added

d17a3ac

Incorrect BigInt equality fixed

3ad7f32

Karatsuba added

2da0648

Incorrect equals fix test Incorrect overflow handling support

altavir requested changes May 11, 2021

View reviewed changes

CommanderTvis requested changes May 11, 2021

View reviewed changes

pklimai reviewed May 11, 2021

View reviewed changes

zhelenskiy added 4 commits May 12, 2021 00:58

empty string parsing fixed

74d1497

Parser optimization

9184bd5

Parsing of decimal values takes 9 immediately digits but not 1.

b4e4749

Placing pow outside the Ring interface.

ed86a31

Karatsuba enhancement

ded0125

zhelenskiy added 2 commits May 12, 2021 13:59

Large addition tests added

1e71f29

UInt can be used in exponent (in pow) now.

8443fac

Unstable API annotation added + inconsistent docs fix + unnecessary B…

1e94538

…igIntField usage removed

altavir requested changes May 13, 2021

View reviewed changes

Reusing of existing power function

c204747

zhelenskiy requested a review from altavir May 13, 2021 19:45

altavir approved these changes May 14, 2021

View reviewed changes

altavir added 2 commits May 14, 2021 09:06

Merge branch 'dev' into dev

a299003

fixes to power

bdb9ce6

altavir merged commit c1b94ff into SciProgCentre:dev May 14, 2021

zhelenskiy commented May 14, 2021

View reviewed changes

altavir mentioned this pull request Apr 11, 2022

0.3.0 #480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Karatsuba added, 2 bugs are fixed #328

Karatsuba added, 2 bugs are fixed #328

zhelenskiy commented May 10, 2021

zhelenskiy commented May 10, 2021

breandan commented May 10, 2021

zhelenskiy commented May 11, 2021

altavir left a comment

altavir May 11, 2021

CommanderTvis May 11, 2021

zhelenskiy May 11, 2021

altavir May 11, 2021

zhelenskiy May 11, 2021

CommanderTvis May 11, 2021

pklimai May 11, 2021

zhelenskiy May 12, 2021

zhelenskiy commented May 12, 2021

zhelenskiy commented May 12, 2021

zhelenskiy commented May 12, 2021

altavir May 13, 2021

altavir May 13, 2021

altavir May 13, 2021

zhelenskiy May 13, 2021 •

edited

Loading

zhelenskiy May 13, 2021

altavir May 13, 2021

zhelenskiy May 13, 2021

zhelenskiy May 13, 2021

altavir May 13, 2021

zhelenskiy May 13, 2021

zhelenskiy May 13, 2021

altavir May 13, 2021

zhelenskiy left a comment

zhelenskiy May 14, 2021

altavir May 14, 2021

Karatsuba added, 2 bugs are fixed #328

Karatsuba added, 2 bugs are fixed #328

Conversation

zhelenskiy commented May 10, 2021

zhelenskiy commented May 10, 2021

breandan commented May 10, 2021

zhelenskiy commented May 11, 2021

altavir 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

zhelenskiy commented May 12, 2021

zhelenskiy commented May 12, 2021

zhelenskiy commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhelenskiy May 13, 2021 • 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

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

zhelenskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhelenskiy May 13, 2021 •

edited

Loading