`Aggregator` dependency injection, `min`/`max`, and `skipNaN` #1108

Jolanrensen · 2025-03-25T12:09:42Z

Helps #961

Refactored Aggregator, splitting it into 3 parts:
- Input, can be supplied by a class implementing AggregatorInputHandler, has 2 implementations
- Aggregation, can be supplied by a class implementing AggregatorAggregationHandler, has 2 implementations
- Handling multiple columns, can be supplied by a class implementing AggregatorMultipleColumnsHandler, has 3 implementations

Together they can instantiate Aggregator in any combination you like :)

Added Aggregator.aggregateByOrNull() functions for selecting aggregators, like min/max, but also median/percentile in the future, with the Sequence.bestBy family of internal functions
added skipNaN option to sum, mean, min, max
Rewrote min/max with correct types (T : Comparable<T & Any>?)
- Decided against allowing mixed number types. Allowing this requires each function to get 3 overloads to avoid ambiguity:
  - <T : Comparable<T & Any>?> for normal comparables
  - <T : Number?> for mixed number types
  - <T> ... where T : Number?, T : Comparable<T & Any?> for normal numbers
- Plus a slight rethink of the selecting aggregator because each min with numbers would then act like a minBy { it.toUnifiedNumber() } as it needs to return unconverted minimal value
- But if it's valueble enough, we can add it
tests

…egation of specific abilities of the aggregator to whichever combination the client chooses. And instead of having to create a new class for each combination, the same function can be used.

…ingAggregationHandler for min/max etc. Added SequenceBestBy.kt helpers

Jolanrensen · 2025-03-31T11:46:11Z

Seems our notebook tests now fail because of https://youtrack.jetbrains.com/issue/KTNB-967/IllegalStateException-null-DefinitelyNotNullType-for-T-exception-while-analyzing-expression

…mberInputHandler

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/statistics/min.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/math/min.kt

...aframe/impl/aggregation/aggregators/multipleColumnsHandlers/TwoStepMultipleColumnsHandler.kt

...rg/jetbrains/kotlinx/dataframe/impl/aggregation/aggregators/inputHandlers/AnyInputHandler.kt

...otlin/org/jetbrains/kotlinx/dataframe/impl/aggregation/aggregators/AggregatorInputHandler.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/max.kt

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/DataColumnType.kt

zaleslaw · 2025-04-01T16:09:00Z

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/Nulls.kt

+@JvmName("isNaWithContract")
+@Suppress("NOTHING_TO_INLINE")
+@OptIn(ExperimentalContracts::class)
+internal inline fun <T : Any?> T.isNA(): Boolean {


isNa -> isMissing() for example values.filterNot { it.isMissing() }, bad previous naming for String for example
"abc".isNA()

maybe... but "NA" is a well documented DataFrame concept by now. It even has a documentation page https://kotlin.github.io/dataframe/nanandna.html. So I think it's reasonable to keep using the name, meaning "Not Available", so "null or NaN". If you have doubts about it, we could create a separate issue though

Not big doubts, I starting to think about NA as about NaN - the problem with closest definitions

Would be nice to have a small KDoc about what NA is 👍 .

Jolanrensen · 2025-04-01T18:08:25Z

Mixed number type proposal #1113

Jolanrensen · 2025-04-02T15:53:11Z

Notebooks failing issue: #1116

Jolanrensen · 2025-04-03T15:18:42Z

Seems I've caused some NoSuchMethodErrors in combination with kandy... Will need to compare the api dump for minuses and add hidden overloads where possible

AndreiKingsley

Very nice! Regarding min/max mixed types: it seems to me that in such functions it is better to tell the user "please cast to a common type T: Comparable." Adding x3 overloads is a huge amount of initial work and it will be even harder to maintain this in the future. I'd keep std lib logic.

AndreiKingsley · 2025-04-04T10:40:44Z

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/Nulls.kt

+@JvmName("isNaWithContract")
+@Suppress("NOTHING_TO_INLINE")
+@OptIn(ExperimentalContracts::class)
+internal inline fun <T : Any?> T.isNA(): Boolean {


Would be nice to have a small KDoc about what NA is 👍 .

AndreiKingsley · 2025-04-04T10:47:02Z

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/mean.kt

@@ -34,242 +34,245 @@ import kotlin.reflect.typeOf

 // region DataColumn

-public fun DataColumn<Number?>.mean(skipNA: Boolean = skipNA_default): Double = Aggregators.mean(skipNA).aggregate(this)
+public fun DataColumn<Number?>.mean(skipNaN: Boolean = skipNaN_default): Double =


The thing I don't like about skipNaN_default - the user doesn't see the actual default value of the arguments. It's a bit annoying or something. And also global variables are the evil 👿 . Do we keep it as a variable on purpose? I'd really like to remove it.

they will :) https://youtrack.jetbrains.com/issue/KTIJ-31662/Parameter-info-Show-value-of-const-default-argument-when-calling-a-function
and yes, we keep it on purpose; it's really good to have a single source of truth for a default value like this. Without this constant it's really easy to create an overload with a different default accidently, so it promotes consistency and predictability of the API.

… Fixed puzzles notebook

Jolanrensen added 5 commits March 21, 2025 13:02

starting min/max

418acfe

wip making Aggregators more modular

dc7502e

Merge branch 'master' into min-max

37a2a1d

switching aggregators fully to sequences

b8efaa0

Rewrote Aggregator to a dependency injection pattern. This allows del…

79be369

…egation of specific abilities of the aggregator to whichever combination the client chooses. And instead of having to create a new class for each combination, the same function can be used.

Jolanrensen mentioned this pull request Mar 25, 2025

☂ Statistics streamlining #961

Open

9 tasks

Jolanrensen added 3 commits March 25, 2025 21:56

DefaultAggregationHandler -> ReducingAggregationHandler. Added Select…

158597d

…ingAggregationHandler for min/max etc. Added SequenceBestBy.kt helpers

Continuing aggregation refactor

3629b35

finished types and aggregateBy for min

6c76219

Jolanrensen force-pushed the min-max branch from c01e4ae to 6c76219 Compare March 27, 2025 13:24

Jolanrensen added 4 commits March 27, 2025 16:06

added skipNaN option for mean, sum, and min

751321e

refactored AggregatorProvider back

2a90300

update max api similar to min

21f4ccc

kdocs, cleaning, and some tiny refactorings

a0279cd

Jolanrensen changed the title ~~Another Aggregator refactor and support for min/max~~ Aggregator dependency injection, min/max, and skipNaN Mar 28, 2025

Jolanrensen changed the title ~~Aggregator dependency injection, min/max, and skipNaN~~ Aggregator dependency injection, min/max, and skipNaN Mar 31, 2025

Jolanrensen added 3 commits March 31, 2025 16:29

added mean tests

2e0875b

enabled some previously broken sum.kt tests and fixed edge case in Nu…

73ceaa8

…mberInputHandler

added more sum tests

87ab8ed

Jolanrensen force-pushed the min-max branch from 48db146 to 87ab8ed Compare March 31, 2025 18:47

added tests and fixed NaN behavior for min/max functions

12d0922

Jolanrensen force-pushed the min-max branch from b4c4e36 to 12d0922 Compare April 1, 2025 14:26

Jolanrensen requested review from zaleslaw, koperagen and AndreiKingsley April 1, 2025 14:27

Jolanrensen marked this pull request as ready for review April 1, 2025 14:27

zaleslaw requested changes Apr 1, 2025

View reviewed changes

Jolanrensen mentioned this pull request Apr 1, 2025

Allow mixed number types for min/max statistics #1113

Open

Jolanrensen added 3 commits April 1, 2025 21:03

unified min and max statistics implementations

36ed245

added note about reflection in input handlers

60319f4

separated parts of min/max tests

b427b6a

Jolanrensen force-pushed the min-max branch from dd095d4 to b427b6a Compare April 2, 2025 12:20

Jolanrensen requested a review from Copilot April 2, 2025 15:17

This comment was marked as resolved.

Sign in to view

Jolanrensen mentioned this pull request Apr 2, 2025

Notebooks fail with T : Comparable<T & Any>? notation #1116

Open

Merge branch 'master' into min-max

973b521

ignore notebook tests if they fail because of issue #1116

6c67050

AndreiKingsley approved these changes Apr 4, 2025

View reviewed changes

Jolanrensen added 2 commits April 4, 2025 13:21

added overloads for binary compatibility with added skipNaN argument.…

348b84b

… Fixed puzzles notebook

adding docs based on feedback

b4fd200

Jolanrensen merged commit 7c63a80 into master Apr 4, 2025
4 of 6 checks passed

Jolanrensen added this to the 1.0.0-Beta1 (0.16) milestone Apr 4, 2025

Jolanrensen added the enhancement New feature or request label Apr 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Aggregator` dependency injection, `min`/`max`, and `skipNaN` #1108

`Aggregator` dependency injection, `min`/`max`, and `skipNaN` #1108

Jolanrensen commented Mar 25, 2025 •

edited

Loading

Jolanrensen commented Mar 31, 2025

zaleslaw Apr 1, 2025

Jolanrensen Apr 1, 2025

zaleslaw Apr 2, 2025

AndreiKingsley Apr 4, 2025

Jolanrensen commented Apr 1, 2025

This comment was marked as resolved.

Jolanrensen commented Apr 2, 2025

Jolanrensen commented Apr 3, 2025

AndreiKingsley left a comment

AndreiKingsley Apr 4, 2025

AndreiKingsley Apr 4, 2025

Jolanrensen Apr 4, 2025

Aggregator dependency injection, min/max, and skipNaN #1108

Aggregator dependency injection, min/max, and skipNaN #1108

Conversation

Jolanrensen commented Mar 25, 2025 • edited Loading

Jolanrensen commented Mar 31, 2025

zaleslaw Apr 1, 2025

Choose a reason for hiding this comment

Jolanrensen Apr 1, 2025

Choose a reason for hiding this comment

zaleslaw Apr 2, 2025

Choose a reason for hiding this comment

AndreiKingsley Apr 4, 2025

Choose a reason for hiding this comment

Jolanrensen commented Apr 1, 2025

This comment was marked as resolved.

Jolanrensen commented Apr 2, 2025

Jolanrensen commented Apr 3, 2025

AndreiKingsley left a comment

Choose a reason for hiding this comment

AndreiKingsley Apr 4, 2025

Choose a reason for hiding this comment

AndreiKingsley Apr 4, 2025

Choose a reason for hiding this comment

Jolanrensen Apr 4, 2025

Choose a reason for hiding this comment

`Aggregator` dependency injection, `min`/`max`, and `skipNaN` #1108

`Aggregator` dependency injection, `min`/`max`, and `skipNaN` #1108

Jolanrensen commented Mar 25, 2025 •

edited

Loading