Median overhaul

[Jolanrensen]

Helps #961

WIP

[Jolanrensen]

https://youtrack.jetbrains.com/issue/KT-76683 is holding me back

[Jolanrensen]

@zaleslaw https://github.com/Kotlin/dataframe/blob/median/plugins/kotlin-dataframe/testData/box/groupBy_median.kt now fails because the return type of median changed to:

comparable -> itself or null
primitive number -> double or null

Shall I ignore the test for now so you can cover it after it's merged?

[zaleslaw]

Yes, you could ignore, I will fix soon

[zaleslaw]

Logic here and next 3 rows is really hard to understand and follow, are you sure, that it covers all the required mixed situation Double.NaN + null, could it be here?

[Jolanrensen]

null cannot occur in this function because of the type T : Comparable<T>. The aggregator integration already filters them out. So yes, it covers all the cases. I think it reads like a sentence: if the type of the values cán be nan and nans are not skipped, then if any is nan, return nan. In all other cases we continue and ignore nans. How would you rewrite it?

[zaleslaw]

Having Any as a return type defeats the purpose of strong typing, do we really need this?

[AndrewKis]

Agree, the should be Number at least, no?

[Jolanrensen]

Median works for any comparable, also Strings, datetimes etc. So Number would be wrong.

The only correct return type would be R where R : Comparable<R>, but since we don't know R, Any comes closest to the actual return type for the string-api. We use the same for the column accessors in describe().

We also cannot return Comparable<Any?> because that would imply you could call df.median("someCol").compareTo(anything) and that will likely break. We also cannot give Comparable<*>/Comparable<Nothing> because a) we cannot return Nothing and b) df.median("someCol").compareTo(anything) will then also break.

We could, however, force users to give a specific type R, how do you like that idea?

[AndrewKis]

We could, however, force users to give a specific type R, how do you like that idea?

Why not, but please write an example first, not sure how it looks.

[Jolanrensen]

sure, something like:

val medianAge: Int = df.median<Int>("age")
val medianLapTime: Double = df.median<Double>("lap1", "lap2", skipNaN = true)

[zaleslaw]

probably debug mode?

[Jolanrensen]

It's just a warning logged right? users can disable it by changing the log level for org.jetbrains.kotlinx.dataframe.math.MedianKt on their end or by converting their columns to Double manually first. It's not a heavy check, so I think debug mode is not needed.

[zaleslaw]

I have a strong feeling, that we need less overloads/median* methods

[AndrewKis]

Are you sure about crossinline here?

[Jolanrensen]

crossline is more efficient than noinline and unfortunately we cannot do it without, so yes.

[AndrewKis]

Agree, the should be Number at least, no?

[Jolanrensen]

I have a strong feeling, that we need less overloads/median* methods

Please enlighten me if you know how :)
We have the following restrictions:

• When we encounter specific number columns, the return type should be Double
• When we encounter other comparables, the return type should be the same as the input
• For float/double columns, we need a skipNaN argument.
• Just like other statistics we have all the X, of/by/for, row, and -orNull overloads for columns, dataframes, pivots, groupBy, and pivotGroupBy. Nothing really groundbreaking here

I already tried to combine overloads where possible, so if the return type of the function does not change depending on the input type (like for DataFrame.medianFor {} -> DataRow) I just use one overload that accepts any comparable, number or not, and has the skipNaN argument. In other cases, where the return type does change, I made two variants; one that returns Double and another that returns T.