https://kotlinlang.org logo
Join Slack
Powered by
<@U5F0TT0UX> Let the <discussion> about the core c...
# mathematics
z
@altavir Let the discussion about the core change be here. I suggested inventing fast power method for Rings.
Here is the current implementation:
Copy code
public fun <T> Ring<T>.pow(base: T, exponent: ULong): T = when {
    this == zero && exponent > 0UL -> zero
    this == one -> base
    this == -one -> powWithoutOptimization(base, exponent % 2UL)
    else -> powWithoutOptimization(base, exponent)
}

private fun <T> Ring<T>.powWithoutOptimization(base: T, exponent: ULong): T = when (exponent) {
    0UL -> one
    1UL -> base
    else -> {
        val pre = powWithoutOptimization(base, exponent shr 1).let { it * it }
        if (exponent and 1UL == 0UL) pre else pre * base
    }
}
I also invented function for the big integer to make it easier to access power:
Copy code
public fun pow(other: ULong): BigInt = BigIntField { pow(this@BigInt, other) }
a
The problem with methods in core algebras is that it will be mandatory for all descendants. It makes kind of sense, still it requires much more thinking. For example, why ULong and not UInt
z
I know and agree about necessity of further discussion of the core changes. As I explained in Github, it is useless for BigInts (because of overflow) but may be useful for other Rings. However, it may be better to create also
Copy code
fun <T> Ring<T>.pow(base: T, exponent: UInt) = pow(base, exponent.toULong())
a
The problem is that onece we add something to the core, it will be really hard to remove it. An extension function could be marked experimental and removed without harm later.
z
Ok, I will add that. Are there any arguments against the current implementation (being extension function, having pow with ulong and with uint that calls ulong version, having function in BigInt that calls Ring version of pow, etc)?
And which annotation do you suggest? 
kotlin.Experimental
is not applicable to target 'top level' function' which 
pow
actually is.
a
UnstableKMathAPI
z
I added it and published the changes
Do I need to do anything else there?
a
I've re-assigned the review so we will do it as soon as all accept changes.
z
Thank you! 😀
a
OK, looks good, but I found out that we already have those extensions in a separate file, so you should just replace implementations in them instead of creating new functions.
Also be sure to resolve conflicts
z
Conflicts of names - of course. I have done everything.
a
Merged. There were some problems there still, but I've fixed them on merge
z
Thank you!
Well, I checked. You replaced Int -> UInt in Field. However, 
Field
can accept negative values. That was even in previous implementation. And there is a check that you still left in code. I haven't checked, but that is probably not compilable now.
a
It was a mistake, I've reverted it
there was a problem in extension, it took this instead of arg. I've fixed it. Also I've changed a long-running random test since it took too much time on native.
z
I mean the following code
(second part)
a
Yeah, it was my mistake, i've reverted it. See the current state in 
dev
z
Also, where can I check the test that was reduced?
a
z
This one. Right?
a
yes. The library is already quite heavy so we can't allow long running tests. Maybe it is a good idea to do a separate module with optional tests later.
z
Ok. (Or it was possible to reduce repeat number to 20 - 50)
a
It is not quite good idea to do fully random tests because it could cause error reproducability issues. It is better to add those tests to examples and run them manually.
z
I expected that to be a stress test
a
Indeed. But stress tests should not be run with unit tests. Otherwise there will be problems with CI. You can add stress tests to examples module as stand-alone programs.
z
Perhaps, it is worth adding a module with stress tests as there is one with benchmarks.
a
good idea
z
And I crated a feature request for the stress-tests: https://github.com/mipt-npm/kmath/issues/335
👍 1
3 Views
Open in Slack
PreviousNext
Powered by