Rework of dfs functions to recursively (changing return type of recurse-able functions) #363

Jolanrensen · 2023-04-22T14:19:04Z

Same as #361 but using a different tactic.

Largest changes:

Streamlined behavior of all, cols, groups etc. such that if it's called on

a SingleColumn<*> containing a single ColumnGroup (such as DataFrame), the operation will run on the children of that ColumnGroup
else, or a normal ColumnSet<*>, it will simply run on the columns inside.
This is enforced using the new allInternal() function combined with isSingleColumnWithGroup. I think this simply enforces expected behavior, since all tests work with it (and break upon a slight modification of this function) and more importantly, the calls seem to make sense. Let me know what you think.

Introducing TransformableColumnSet and TransformableSingleColumn. A wrapper around the original ones that provide a way to transformResolve as well as the normal resolve. transformResolve takes a ColumnSetTransformer that can get executed before resolving the ColumnSet or SingleColumn. A ColumnSetTransformer contains implementations for both transform and transformSingle, similar to the functions in Utils.kt. Currently, the only implementation of this is RecursivelyTransformer, which is configured and created in the TransformableColumnSet<>.recursively() calls.
Not all ColumnSets are transformable. Functions that allow reconfiguration with a ColumnSetTransformer need to explicitly state that in their return type. In principle, any function that uses transform {} can automatically opt-in to the TransformableColumnSet return-type if that makes sense for them. Currently, I added the ability to cols {}, all(), groups {}, children {}, filter {}, nameContains(), endsWith(), startsWith(), except(), and withoutNulls().
I added TransformableSingleColumn to first {}, last {}, and single {} (which uses singleWithTransformerImpl to pass on the transformer to the right place).
Of course, if other functions are applicable to modification calls like .recursively() I'd gladly hear it.

.recursively() / .rec() is the replacement of dfs {}, dfsOf<>{}, and allDfs(). because the name was confusing. I put deprecations in place and replaced all calls in the library to recursively. Some things to note:

dfs {}, called on either a ColumnSet or SingleColumn would exclude the top-level of columns but still include column groups in the end-result, so a proper replacement of that call would be cols {}.recursively() if called on a SingleColumn and cols {}.recursively(includeTopLevel = false) if called on a normal ColumnSet. Of course rec() can be used as shorthand.
dfsOf<>(), is replaced similarly to either colsOf<>().rec() or colsOf<>().rec(includeTopLevel = false).
allDfs() defaulted includeGroups to false, so be careful, since this is now true. It can be replaced either with all().recursively() or allRecursively() (or allRec()), again taking into account includeTopLevel and includeGroups.

Other smaller changes:

Renamed occurrences of "Dfs" where "DataFrames" was meant as such, to avoid confusion
Renamed:
- inner functions named dfs to depthFirstSearch where depth first search was actually meant
- TreeNode.dfs {} -> TreeNode.allChildren {} (+ other dfs functions)
- TreeNode.topDfs {} -> TreeNode.topmostChildren {} (+ other topDfs functions)
- Iterable<ColumnWithPath<*>>.dfs() -> flattenRecursively()
- Some unavoidable linting in the files I touched
Updated korro to 0.1.4
Removed ColumnPath.something overloads in ColumnsSelectionDsl since they are handled by SingleColumn<*>
Added docs to ColumnSet and SingleColumn
Reworked tests for ColumnsSelectionDsl
Added recursively tests

…ively/rec

… tests

…-rename branch.

…zed versions on which transformers like "recursively" can be called.

topDfs -> topmostChildren top -> roots

# Conflicts: # examples/jupyter-notebooks/titanic/Titanic.ipynb

… rec

# Conflicts: # core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/Modify.kt # core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/TestBase.kt # core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/Modify.kt # core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/samples/api/TestBase.kt # docs/StardustDocs/topics/ColumnSelectors.md

core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/DataColumn.kt

...generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt

zaleslaw · 2023-05-03T15:53:24Z

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

        while (node != null) {
            node.data = null
            node = node.parent
        }
    }
-    val dfs = fullTree.topDfs { it.data != null }
-    return dfs.map { it.data!!.addPath(it.pathFromRoot()) }
+    val subtrees = fullTree.topmostChildren { it.data != null }


topmostChildren?
topDFS is a bad name, but topmostChildren leads me to the mist again

what would you suggest?

As my doc explains it takes the tree and returns first the children, closest to the root of the tree, where the predicate holds. It doesn't matter whether the children of those nodes also hold, those are ignored. So since we now have allChildren { something == true }, I thought topmostChildren { something == true }. It can also be viewed as subtree-roots where the predicate holds.

@koperagen Any suggestions? :)

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

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

core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/html.kt

core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/ColumnsSelectionDsl.kt

koperagen · 2023-05-03T17:16:21Z

core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/replace.kt

@@ -37,7 +37,7 @@ public fun <T, C> DataFrame<T>.replace(columns: Iterable<ColumnReference<C>>): R

 public fun <T> DataFrame<T>.replaceAll(
    vararg valuePairs: Pair<Any?, Any?>,
-    columns: ColumnsSelector<T, *> = { allDfs() },
+    columns: ColumnsSelector<T, *> = { all().recursively(includeGroups = false) },


Why has the default value for includeGroups changed? I would argue that one of the main usages of dfsOf is to convert / replace columns with some type and groups are not needed

Is it correct that it's because of unification of overloads?

Yes, by default groups were included in dfs {}, but excluded in allDfs() if I recall correctly. I had to choose a sensible default. But if you use colsOf<Int>().rec(), then includeGroups irrelevant anyways, since Int is not a Group.

Hmm, this is interesting and a bit confusing. I think this parameter only exists for allDfs. For normal dfs it would be dfs { !it.isColumnGroup() }. Now it's also present for rec, recursively. What if you remove it from them and leave only for allRec with the same default value (true)?

.rec() can not only be called on all(), you can also call first { }.rec(includeGroups = false), so I think keeping it in rec/recursively too would be best.

I don't think we have a function that can give all valueColumns right? If we did we could have valueCols {}.recursively() or valueColsRecursively() but that might get a bit long.

Plus it's not just ColumnGroups that get included right? also FrameColumns and ValueColumns. So, just all of them. I think that's a good default

@koperagen Actually I do want to add valueCols {} and frameCols {} since we already have groups {} as well. Probably in another PR though. Then you can call valueCols().rec() if you want. Sounds okay?

Could be a nice addition, not 100% related to this problem though. For this allRecursively function with previous default, something like "allLeafs" might fit better. I think it was named like that before and renamed later

We could also take it in another direction eventually, similar to colsOf<Type>() have colsOf(vararg ColumnKind) {}.rec(), that might be clearer compared to users having to figure out what a "leaf" means.

...erated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/tree/TreeNode.kt

docs/StardustDocs/topics/ColumnSelectors.md

# Conflicts: # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/convert.kt # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/describe.kt # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/inferType.kt # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/reorder.kt # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/columns/ColumnWithPath.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/describe.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/inferType.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/reorder.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/columns/ColumnWithPath.kt

koperagen · 2023-05-05T11:05:34Z

Another thing that i noticed now is that it's hard to tell what rec does for first { it.type == typeOf<Int> }.rec(). Will it give the same result as dfs { it.type == typeOf<Int> }.first()?

# Conflicts: # docs/StardustDocs/topics/ColumnSelectors.md # gradle/libs.versions.toml

Jolanrensen · 2023-05-05T12:16:16Z

Another thing that i noticed now is that it's hard to tell what rec does for first { it.type == typeOf<Int> }.rec(). Will it give the same result as dfs { it.type == typeOf<Int> }.first()?

Yes, it works the same. You can read it like a sentence: "Select the first column of type Int recursively", which sort of makes sense.
Recursively modifies the call before such that it runs on all columns recursively instead of just on the top-level.

Basically:
allRec().first { it.type == typeOf<Int>() } == first { it.type == typeOf<Int>() }.rec()

docs/StardustDocs/topics/addDf.md

# Conflicts: # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/rename.kt # core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/flatten.kt # core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/rename.kt # core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/flatten.kt # core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt

Jolanrensen added 14 commits April 13, 2023 18:06

ground work for recursively()

5e014f4

erasing type of transform functions

dac8c17

transform: (ColumnSet<*>) -> ColumnSet<*>

6ae692d

SingleColumn<*> in ColumnSelectionDsl. reworked ColumnSelectionDsl tests

d1a84a0

Merge branch 'master' into dfs-rename2

0e57b94

Merge branch 'master' into dfs-rename2

8b0b8a3

poc implemented, needs more tests

e9dfab9

fixed duplicate columns, needs more tests

8fffbd1

seems to be working! In the process of converting dfs usage to recurs…

25d4821

…ively/rec

converted most if not all dfs usage to recursively

d1f584d

docs for cols and all

149e96f

first {}.recursively now seems to work. Why, I'm not sure. Needs more…

da89e93

… tests

getAt

2d08793

3rd attempt, now with different return-type, similar to the first dfs…

e3a115b

…-rename branch.

Jolanrensen added the enhancement New feature or request label Apr 22, 2023

Jolanrensen added this to the 0.11.0 milestone Apr 22, 2023

Jolanrensen added 4 commits April 22, 2023 18:01

children

2dddcc0

TransformableColumnSet and TransformableSingleColumn are now generali…

a47f67e

…zed versions on which transformers like "recursively" can be called.

dfs -> allChildren

4bf8219

topDfs -> topmostChildren top -> roots

Merge branch 'master' into dfs-rename3

efb8a04

# Conflicts: # examples/jupyter-notebooks/titanic/Titanic.ipynb

Jolanrensen mentioned this pull request Apr 25, 2023

Rework of dfs functions to recursively #361

Closed

Jolanrensen added 4 commits April 25, 2023 20:43

adding allRecursively again, docs and cleaning

f94b252

removed ColumnPath overloads, fixed groups, added docs for allRec and…

7c41875

… rec

linting

8c16478

updated korro correctly

d79ed97

Jolanrensen marked this pull request as ready for review April 26, 2023 13:16

Jolanrensen requested a review from koperagen April 26, 2023 13:16

Jolanrensen added 2 commits April 26, 2023 16:56

merged with master and updated docs to include allRecursively

f49f0cd

Jolanrensen requested a review from zaleslaw May 2, 2023 10:36

zaleslaw requested changes May 3, 2023

View reviewed changes

koperagen reviewed May 3, 2023

View reviewed changes

...erated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/tree/TreeNode.kt Show resolved Hide resolved

koperagen reviewed May 3, 2023

View reviewed changes

docs/StardustDocs/topics/ColumnSelectors.md Outdated Show resolved Hide resolved

Jolanrensen added 2 commits May 4, 2023 14:41

updating after first round of reviews

29975f5

Jolanrensen requested review from zaleslaw and koperagen May 4, 2023 14:25

Jolanrensen added 4 commits May 5, 2023 13:21

made renameToCamelCase clearer

b4484f5

removing includeGroups param from recursively

06ccc7b

Merge branch 'master' into dfs-rename3

95818f5

# Conflicts: # docs/StardustDocs/topics/ColumnSelectors.md # gradle/libs.versions.toml

updating with korro

3aa7c0d

koperagen reviewed May 5, 2023

View reviewed changes

docs/StardustDocs/topics/addDf.md Show resolved Hide resolved

Jolanrensen added 2 commits May 5, 2023 17:03

added auto add-to-git for copySamplesOutputs task, reran korro

b42cc5d

fixed korro deleting snippets files at a rerun

b085905

Jolanrensen mentioned this pull request May 8, 2023

Rename dfs function #333

Closed

removed allRecursively and boolean arguments for recursively

d21934b

Jolanrensen requested a review from koperagen May 10, 2023 10:17

Jolanrensen mentioned this pull request May 10, 2023

ColumnsSelection DSL overhaul #372

Merged

33 tasks

Jolanrensen added 3 commits May 10, 2023 18:26

Merge branch 'master' into dfs-rename3

9f40e89

updated from master

9fdd5e3

Jolanrensen merged commit c3031c4 into master May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of dfs functions to recursively (changing return type of recurse-able functions) #363

Rework of dfs functions to recursively (changing return type of recurse-able functions) #363

Jolanrensen commented Apr 22, 2023 •

edited

zaleslaw May 3, 2023

Jolanrensen May 4, 2023

Jolanrensen May 4, 2023

Jolanrensen May 8, 2023

koperagen May 3, 2023 •

edited

koperagen May 3, 2023

Jolanrensen May 4, 2023 •

edited

koperagen May 4, 2023 •

edited

Jolanrensen May 4, 2023

Jolanrensen May 8, 2023

Jolanrensen May 8, 2023

Jolanrensen May 9, 2023

koperagen May 9, 2023 •

edited

Jolanrensen May 10, 2023 •

edited

koperagen commented May 5, 2023

Jolanrensen commented May 5, 2023 •

edited

Rework of dfs functions to recursively (changing return type of recurse-able functions) #363

Rework of dfs functions to recursively (changing return type of recurse-able functions) #363

Conversation

Jolanrensen commented Apr 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koperagen May 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jolanrensen May 4, 2023 • edited

Choose a reason for hiding this comment

koperagen May 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koperagen May 9, 2023 • edited

Choose a reason for hiding this comment

Jolanrensen May 10, 2023 • edited

Choose a reason for hiding this comment

koperagen commented May 5, 2023

Jolanrensen commented May 5, 2023 • edited

Jolanrensen commented Apr 22, 2023 •

edited

koperagen May 3, 2023 •

edited

Jolanrensen May 4, 2023 •

edited

koperagen May 4, 2023 •

edited

koperagen May 9, 2023 •

edited

Jolanrensen May 10, 2023 •

edited

Jolanrensen commented May 5, 2023 •

edited