kyonifer / koma

A scientific computing library for Kotlin. https://kyonifer.github.io/koma
Other
270 stars 23 forks source link

Cleanup workaround code for crossplatform K/Native K/Js K/Java that is no longer necessary #41

Closed LouisCAD closed 6 years ago

LouisCAD commented 6 years ago

Hi, I saw you redefined PI an E here. But doing so is pointless since the val you defined (could have been a const val BTW) is of type Double, which doesn't have as much precision as the digits you wrote. Kotlin's 1.2 math package already includes these constants (which forwards to JVM constants that already have the max precision achievable for type Double).

To prove what I'm saying, here's a snippet that runs without Exception. You can try it on try.kotl.in

typealias BlaBla = Array<String>

fun main(argsBlaBla: BlaBla) {

    val tooLongPiForDouble = 3.1415926535897932384626433832795028841971693993751058209749445923078164062
    check(tooLongPiForDouble == 3.14159265358979323846)
    check(tooLongPiForDouble == Math.PI)
    check(Math.PI == kotlin.math.PI)
    check(kotlin.math.PI == 3.14159265358979323846)
    println(tooLongPiForDouble)
    println(kotlin.math.PI)
}
kyonifer commented 6 years ago

Thanks for the report.

Please note that this library predates 1.2's math package (1.2 was only released 2 days ago) and also predates kotlin's multiplatform support (updating to support that is #38). Indeed, koma was started while Kotlin/JVM proper was still in beta. The redefinition of constants wasn't an attempt at getting more precision than double's are capable of having, but rather an attempt at having those constants defined in one place for all three platforms, which wasnt available from Kotlin up until yesterday. And having some extra decimals didn't seem to hurt anything.

Now that multiplatform is becoming more stable (and as you note 1.2 is stable) probably a lot of the polyfills and platform specific code isn't needed, including these constants. For example the js support includes workarounds for the fact that kotlin.math didn't exist, and the native support fills in a bunch of missing stuff like missing collection extensions that may be included directly in kotlin/native now (that support was written circa kotlin/native 0.1). I also had to dump the @JsName / @JvmName et al annotations from the kotlin source into koma, because otherwise you couldn't have those in the common language code. It looks like that one is still an unresolved issue.

So there's definitely some cleanup needed here, specifically to remove code that was originally a workaround but isn't needed anymore. I was planning a larger refactor of the project to support multiplatform builds instead of conditional compilation in #38, and during that migration I'd hopefully be able to axe all the srcjs/srcjvm stuff (not native yet, sadly), as well as clean up unnecessary duplication in the rng code and things like the PI/E constants.

Alternatively, feel free to open PRs to remove specific pieces of now-unnecessary code as you find them. I'm not sure exactly when I'll have time to resolve #38, or if it makes sense to wait until native is supported to do it (right now I could only do a js/jvm multiplatform project, with conditional compilation required for native still).

kyonifer commented 6 years ago

Renamed this issue to track progress on cleaning up all of the stale crossplatform code.

kyonifer commented 6 years ago

I did a couple of sweeps while adding multiplatform project support, hopefully clearing out most of the old workaround code. Please feel free to open issues if you find any further cruft that I missed.