Skip to content

Replaced concat_horizontal with concat - #1334

Merged
Vincent-Maladiere merged 15 commits into
skrub-data:mainfrom
pvprajwal:update-concat
Apr 25, 2025
Merged

Vincent-Maladiere merged 15 commits into
skrub-data:mainfrom
pvprajwal:update-concat

Conversation

@pvprajwal

Copy link
Copy Markdown
Contributor

Fixes #1330

Replaced concat_horizontal with concat. The axis parameter can be used to specify whether to concatenate horizontally or vertically.
axis=1 or axis='columns' can be used for horizontal
axis=0 or axis='index' can be used for horizontal

replaced `skrub.concat_horizontal` with `skrub.concat` to use vertical or horizontal concatenation where setting the axis would copy the behavior of `pd.concat`
added tests for vertical concatenation and updated docstrings
- added tests for axis not defined
- updated changes.rst

@Vincent-Maladiere Vincent-Maladiere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @pvprajwal, thank you for this PR! Here are a couple of remarks

Comment thread skrub/_dataframe/_common.py Outdated
Comment thread skrub/_dataframe/_common.py Outdated
Comment thread skrub/_dataframe/_common.py Outdated
Comment thread skrub/_dataframe/_common.py Outdated
Comment thread skrub/_dataframe/_common.py Outdated
Comment thread skrub/_expressions/_skrub_namespace.py Outdated
Comment thread skrub/_expressions/_skrub_namespace.py Outdated
Comment thread skrub/_expressions/tests/test_evaluation.py Outdated
Comment thread CHANGES.rst Outdated
Comment thread CHANGES.rst Outdated
pvprajwal and others added 8 commits April 22, 2025 22:39
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>
concatenating vertically now ignores the original indexes
@jeromedockes

jeromedockes commented Apr 22, 2025 •

Copy link
Copy Markdown
Member

Thanks a lot for working on this @pvprajwal and welcome to skrub 🎉 ! this is great, you took on a challenging issue for your first contribution and the result is looking good! I agree with @Vincent-Maladiere 's point that we should not support axis='index' or axis='columns' because 'index' only makes sense for pandas. it's looking good otherwise :) 🚀

@pvprajwal

Copy link
Copy Markdown
Contributor Author

Thanks so much @jeromedockes ! I am glad to be part of the skrub community. I appreciate the feedback from you and @Vincent-Maladiere .

Regarding axis='horizontal'/'vertical', do we want to support that?

@Vincent-Maladiere

Copy link
Copy Markdown
Member

It's our pleasure @pvprajwal, thank you for contributing! If we don't support axis='index'/'column', then I guess we shouldn't support axis='horizontal'/'vertical' either, WDYT?

@jeromedockes

Copy link
Copy Markdown
Member

let's go with just axis=0 or axis=1 for now, it's easier to add stuff later than to remove it

@pvprajwal

Copy link
Copy Markdown
Contributor Author

Alright, I'll update the code.

the axis parameter in concat now takes only 0/1 values.
@pvprajwal
pvprajwal requested a review from jeromedockes April 24, 2025 14:06

@jeromedockes jeromedockes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @pvprajwal !! super useful

@Vincent-Maladiere Vincent-Maladiere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks @pvprajwal :)

@Vincent-Maladiere

Copy link
Copy Markdown
Member

Merging after the conflict have been sorted out

@pvprajwal

Copy link
Copy Markdown
Contributor Author

conflicts resolved @Vincent-Maladiere .

@Vincent-Maladiere
Vincent-Maladiere merged commit b4ca0f0 into skrub-data:main Apr 25, 2025
@GaelVaroquaux

Copy link
Copy Markdown
Member

Thanks a lot, @pvprajwal ! Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace concat_horizontal with concat

4 participants