-
Notifications
You must be signed in to change notification settings - Fork 75
Move to(Start) keeping inside group #1489
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
base: master
Are you sure you want to change the base?
Move to(Start) keeping inside group #1489
Conversation
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt
Outdated
Show resolved
Hide resolved
core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/move.kt
Outdated
Show resolved
Hide resolved
Oh, I see we also have these shortcut functions:
Could you do the same there as with |
Good Morning, I implemented a new logic as suggested in #1489 (comment). |
about #1489 (comment) , Now there are both toStart() and toEnd(),
.to() implements yet these shortcuts because it is possible to create a MoveClause with both ColumnsSelector<T, *> |
* @param [insideGroup] If true, selected columns will be moved to the start remaining inside their group, | ||
* else they will be moved to the start on top level. | ||
*/ | ||
public fun <T, C> MoveClause<T, C>.toStart(insideGroup: Boolean): DataFrame<T> = to(0, insideGroup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, please keep
@Refine
@Interpretable("MoveToStart0")
I will add compiler plugin for this variant of the function. It can use the same interpreter as toStart()
, but it needs to be merged first and published as dev version so we can test it works in the compiler plugin in the Kotlin repo.
@CarloMariaProietti I'm not sure what you mean, public fun <T> DataFrame<T>.moveTo(newColumnIndex: Int, columns: ColumnsSelector<T, *>): DataFrame<T> =
move(columns).to(newColumnIndex)
public fun <T> DataFrame<T>.moveTo(newColumnIndex: Int, insideGroup: Boolean, columns: ColumnsSelector<T, *>): DataFrame<T> =
move(columns).to(newColumnIndex, insideGroup) and public fun <T> DataFrame<T>.moveTo(newColumnIndex: Int, vararg columns: String): DataFrame<T> =
moveTo(newColumnIndex) { columns.toColumnSet() }
public fun <T> DataFrame<T>.moveTo(newColumnIndex: Int, insideGroup: Boolean, vararg columns: String): DataFrame<T> =
moveTo(newColumnIndex, insideGroup) { columns.toColumnSet() } right? If you want, we could also add these in another PR of course. |
val parentOfFirst = columnsToMoveParents.first() | ||
if (columnsToMoveParents.any { it != parentOfFirst }) { | ||
throw IllegalArgumentException( | ||
"Cannot move columns with different parent to an index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please specify "insideGroup", as it would be fine to move them with insideGroup = false
val sonsWithFullPaths = sons.columns().map { parentPath + it.path } | ||
val intermediateDf = df.removeImpl { sonsWithFullPaths.toColumnSet() } | ||
// move sons and reinsert them | ||
val columnsToMoveWithReducedPath = columnsToMove.map { it.path.last(it.path.size - parentPath.size).toPath() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if all columns here have the same parent... isn't their "reduced path" simply their name?
return moveTo(columnIndex) | ||
} | ||
|
||
// logic: remove columns to move and their siblings (from this point, sons), apply them moveTo, reinsert them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great you made it work! But I have a feeling this is way more complicated than it needs to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling was right, I managed to pass all your tests with this logic:
val columnsToMoveNames = columnsToMove.map { it.name() }
return df.replace { parentOfFirst.asColumnGroup() }.with {
it.asDataFrame()
.move { columnsToMoveNames.toColumnSet() }.to(columnIndex)
.asColumnGroup(it.name())
}
Let me explain what's done here:
- we take the column names of those we want to move. This is enough, because we already checked they have the same parent.
- since we now only need to move columns around on the same level, this is just as easy as
moveTo()
on the top level. We can usereplace { parent }
to get access to just the parent group - then, treating the column group as a DataFrame, we can simply call
move {}.to()
- and convert it back to a column group with the right name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation, your solution is much more smooth. I ignored replace { } so i had to remove the level and reinsert it... If you agree, I'll paste this in the code.
val columnsToMoveParents = columnsToMove.map { it.path.dropLast() } | ||
val parentOfFirst = columnsToMoveParents.first() | ||
if (columnsToMoveParents.any { it != parentOfFirst }) { | ||
throw IllegalArgumentException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion from my colleague: Can you create your own exception that implements DataFrameError
? that way we can catch it in the compiler plugin later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
*/ | ||
@Refine | ||
@Interpretable("MoveToStart1") | ||
public fun <T> DataFrame<T>.moveToStart(columns: ColumnsSelector<T, *>, insideGroup: Boolean): DataFrame<T> = move(columns).toStart(insideGroup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure columns
is the last argument, else the lambda outside parentheses doesn't work. For consistency, you can do the same in the vararg String overloads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that way people can write:
df.moveToStart { cols }
// and
df.moveToStart(insideGroup = true) { cols }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I fixed also moveToEnd
About #1255
It comes with docs and a test case