z

    Zhelenskiy

    1 year ago
    @altavir Let the discussion about the core change be here. I suggested inventing fast power method for Rings.
    Here is the current implementation:
    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:
    public fun pow(other: ULong): BigInt = BigIntField { pow(this@BigInt, other) }
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    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
    fun <T> Ring<T>.pow(base: T, exponent: UInt) = pow(base, exponent.toULong())
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    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.
    altavir

    altavir

    1 year ago
    UnstableKMathAPI
    z

    Zhelenskiy

    1 year ago
    I added it and published the changes
    Do I need to do anything else there?
    altavir

    altavir

    1 year ago
    I've re-assigned the review so we will do it as soon as all accept changes.
    z

    Zhelenskiy

    1 year ago
    Thank you! 😀
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    Conflicts of names - of course. I have done everything.
    altavir

    altavir

    1 year ago
    Merged. There were some problems there still, but I've fixed them on merge
    z

    Zhelenskiy

    1 year ago
    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.
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    I mean the following code
    (second part)
    altavir

    altavir

    1 year ago
    Yeah, it was my mistake, i've reverted it. See the current state in
    dev
    z

    Zhelenskiy

    1 year ago
    Also, where can I check the test that was reduced?
    altavir

    altavir

    1 year ago
    z

    Zhelenskiy

    1 year ago
    This one. Right?
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    Ok. (Or it was possible to reduce repeat number to 20 - 50)
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    I expected that to be a stress test
    altavir

    altavir

    1 year ago
    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

    Zhelenskiy

    1 year ago
    Perhaps, it is worth adding a module with stress tests as there is one with benchmarks.
    altavir

    altavir

    1 year ago
    good idea
    z

    Zhelenskiy

    1 year ago
    And I crated a feature request for the stress-tests: https://github.com/mipt-npm/kmath/issues/335