Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mk.math.argMax doesn't discriminate doubles between 0 and 1 #16

Closed
occultus73 opened this issue Mar 11, 2021 · 3 comments
Closed

mk.math.argMax doesn't discriminate doubles between 0 and 1 #16

occultus73 opened this issue Mar 11, 2021 · 3 comments
Assignees
Labels
bug Something isn't working jvm An issue/PR related to JVM
Milestone

Comments

@occultus73
Copy link

I believe I have found a bug with the argMax method. For a Double D1Array like this:
[0.008830892605042162, 0.7638366187431083, 0.2120482416220285, -0.1376875285688708, 0.04677604134217575, -0.11842322358629093, -0.040132636896546836, -0.26975741918728957, 0.006022397547541469, 0.2745433876411484, -0.1882133697860493, 0.0715491465942086, -0.0728390325349591, -0.04587817955186226, -0.3298069957413544, 0.3580707507112211, -0.07260259350541257, -0.0784403405630531, -0.021586318003611993, 0.0557500172047746, 0.02325952046399335, -0.005522260502330234, -0.06656836136167943, 0.16638479724410082, -0.05508238956796159, 0.014393084997982114, 0.03480731825314662, 0.11760367945163475, -0.037000109020572255, -0.053639491632357504, -0.18112447733588335, -0.0024963482748966426, -0.13649951751668346]
It will return zero every time. Only when one of the numbers is above 1, does it return the index of that number, instead of zero.

Presumably the problem is these doubles are being truncated and compared as integers under the hood.

@occultus73 occultus73 changed the title mk.math.argMax doesn't discriminate between doubles between 0 and 1 mk.math.argMax doesn't discriminate doubles between 0 and 1 Mar 11, 2021
@occultus73
Copy link
Author

occultus73 commented Mar 11, 2021

The culprit appears to be this method:

//TODO(create module utils)
/*internal*/ public operator fun <T : Number> Number.compareTo(other: T): Int = (this - other).toInt()

In org.jetbrains.kotlinx.multik.ndarray.data. It overrides the less-than comparison operator used in the argMax algorithim.

@occultus73
Copy link
Author

Gosh that's a surprisingly tricky problem, isn't it? Ideally you could just put in:
(this - other).sign.toInt()

but it's not provided for shorts and bytes:
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.math/sign.html

@devcrocod devcrocod self-assigned this Mar 15, 2021
@devcrocod devcrocod added bug Something isn't working jvm An issue/PR related to JVM labels Mar 15, 2021
@devcrocod devcrocod added this to the 0.0.2 milestone Mar 15, 2021
@devcrocod
Copy link
Collaborator

Thanks for finding this bug.

Kotlin reuses java Double.compare(double d1, double d2), so I think in this case we can use a cast and call compareTo on a specific type. Since there is still a case with NaN.

@devcrocod devcrocod mentioned this issue Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working jvm An issue/PR related to JVM
Projects
None yet
Development

No branches or pull requests

2 participants