Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BUG] Modify concatenate_columns ignore_empty output #1166

Draft
wants to merge 10 commits into
base: dev
Choose a base branch
from
Draft

[BUG] Modify concatenate_columns ignore_empty output #1166

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
41025d8
Modify ignore_empty output
Fu-Jie Sep 8, 2022
d74a829
Modify ignore_empty output
Fu-Jie Sep 8, 2022
f012b9d
solve doc format
Fu-Jie Sep 8, 2022
adaaf36
Update AUTHORS.md
Fu-Jie Sep 13, 2022
30c1f3c
Update janitor/functions/concatenate_columns.py
Fu-Jie Sep 13, 2022
a25d983
Update concatenate_columns.py
Fu-Jie Sep 14, 2022
1423129
Merge branch 'dev' into solve-ignore-empty
ericmjl Nov 3, 2022
08fe78c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 3, 2022
a29a463
Merge branch 'dev' into solve-ignore-empty
samukweku Nov 28, 2022
66ea4c0
Update CHANGELOG.md
samukweku Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,4 @@ Contributors
- [@ethompsy](https://github.com/ethompsy) | [contributions](https://github.com/pyjanitor-devs/pyjanitor/issues?q=is%3Aclosed+mentions%3Aethompsy)
- [@apatao](https://github.com/apatao) | [contributions](https://github.com/pyjanitor-devs/pyjanitor/issues?q=is%3Aclosed+mentions%3Aapatao)
- [@OdinTech3](https://github.com/OdinTech3) | [contributions](https://github.com/pyjanitor-devs/pyjanitor/pull/1094)
- [@Fu-Jie](https://github.com/Fu-Jie) | [contributions](https://github.com/pyjanitor-devs/pyjanitor/issues/1164)
Fu-Jie marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
- [ENH] Enable `encode_categorical` handle 2 (or more ) dimensions array. PR #1153 @Zeroto521
- [ENH] Faster computation for a single non-equi join, with a numba engine. Issue #1102 @samukweku
- [INF] Cancel old workflow runs via Github Action `concurrency`. PR #1161 @Zeroto521
- [BUG] Modify ignore_empty output in `concatenate_columns`. PR #1164 @Fu-Jie

## [v0.23.1] - 2022-05-03

Expand Down
5 changes: 4 additions & 1 deletion janitor/functions/concatenate_columns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ def concatenate_columns(
raise JanitorError("At least two columns must be specified")

Fu-Jie marked this conversation as resolved.
Show resolved Hide resolved
df[new_column_name] = (
df[column_names].astype(str).fillna("").agg(sep.join, axis=1)
df[column_names]
.astype(str)
.replace(["NaT", "nan", "<NA>"], "")
.agg(sep.join, axis=1)
Copy link
Member

Choose a reason for hiding this comment

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

We might want an argument here to toggle between old and new behaviours. Would you be open to doing so?

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, the translation software I used may not have described it clearly。
For ignoring null values, my idea comes from the implementation of Excel, feeling that the implementation of Excel is more in line with the actual use
https://support.microsoft.com/en-us/office/textjoin-function-357b449a-ec91-49d0-80c3-0e8fc845691c

Copy link
Author

@Fu-Jie Fu-Jie Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ericmjl
It is my understanding that if null values are not ignored, then this is more reasonable.

Pseudo-code

split = ','
if ignore_empty =  False   then  1,2,pd.NA  ->  1,2,
if ignore_empty =  True    then  1,2,pd.NA  ->  1,2

)

if ignore_empty:
Expand Down
2 changes: 1 addition & 1 deletion tests/functions/test_concatenate_columns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def test_concatenate_columns_null_values(missingdata_df):
new_column_name="index",
ignore_empty=True,
)
expected_values = ["1.0-1", "2.0-2", "nan-3"] * 3
expected_values = ["1.0-1", "2.0-2", "3"] * 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update the docstrings for the test.

assert expected_values == df["index"].tolist()


Copy link
Contributor

Choose a reason for hiding this comment

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

I also think it might be worth writing a test merging a custom dataframe with a float column (NaN), a datetime column (NaT) and a string column (None/NA?).

And assert the expected output accordingly.

Then, mention this PR or the attached issue in the test docstring as well, please.

Expand Down