Skip to content

Only skip empty columns if skipna=True #242

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

Open
Pierlou wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Only skip empty columns if skipna=True #242

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1abd6c1
fix: only skip NA values if requested
Pierlou Apr 7, 2026
c36a444
chore: lint
Pierlou Apr 7, 2026
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
7 changes: 5 additions & 2 deletions csv_detective/parsing/columns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,10 @@ def build_remaining_tests_per_col(return_table: pd.DataFrame) -> dict[str, list[
for col in table.columns
}
handle_empty_columns(return_table)
empty_cols = {col for col in table.columns if table[col].dropna().empty}
empty_cols = (
# if NA values should be considered valid, then we can skip empty columns
{col for col in table.columns if table[col].isna().all()} if skipna else {}
Copy link
Copy Markdown
Contributor

@ThibaudDauce ThibaudDauce Apr 7, 2026

Choose a reason for hiding this comment

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

empty_cols is also used to restore tests for columns that are all-NA in the first chunk but have data in later chunks (lines 222-228). Setting it to {} when skipna=False breaks that restoration mechanism — those columns will never be tested even if later batches contain data.

I have a local test I can push on your branch to expose this bug (and avoid it in the futur?)

Copy link
Copy Markdown
Collaborator Author

@Pierlou Pierlou Apr 7, 2026

Choose a reason for hiding this comment

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

I am not sure I see the problem: setting empty_cols to {} here is the same as considering all columns have non-NA values (which could happen), and the rest of the function works the same afterwards. We'll never enter the loop at line 222, and rebuild remaining_tests_per_col at line 250, at the end of the batch

)
remaining_tests_per_col = build_remaining_tests_per_col(return_table)

# hashing rows to get nb_duplicates
Expand Down Expand Up @@ -217,7 +220,7 @@ def build_remaining_tests_per_col(return_table: pd.DataFrame) -> dict[str, list[
fill_value=0,
)
for col in list(empty_cols):
if not batch[col].dropna().empty:
if not batch[col].isna().all():
Comment thread
Pierlou marked this conversation as resolved.
empty_cols.discard(col)
remaining_tests_per_col[col] = [
fmt_label
Expand Down