Skip to content

fix: drop events whose category is null instead of emitting bare-prefix codes - #10

Merged
burkh4rt merged 1 commit into
masterfrom
fix/null-category-codes
Jun 10, 2026
Merged

fix: drop events whose category is null instead of emitting bare-prefix codes#10
burkh4rt merged 1 commit into
masterfrom
fix/null-category-codes

Conversation

@brettbj

@brettbj brettbj commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

How to recreate

import polars as pl
df = pl.DataFrame({"lab_category": [None, "Sodium Level"]})
df.select(pl.concat_str([pl.lit("LAB-RES"), pl.col("lab_category")],
                        separator="//", ignore_nulls=True).alias("code")).drop_nulls()
# -> codes ['LAB-RES', 'LAB-RES//sodium_level']  (the null row survives!)

Why it's a bug
get_entry builds codes with concat_str(..., ignore_nulls=True) so a null category collapses to just the prefix (e.g. LAB-RES) instead of null. The subsequent drop_nulls(subset=["code"]) therefore keeps these rows, and null categories — common in real CLIF *_category columns — enter the vocabulary as a meaningless prefix-only token with whatever numeric value the row carried.

How it was fixed
Wrap the concat in pl.when(pl.col(code).is_not_null()).then(...) so rows with a null category become null codes and are removed by the existing drop_nulls. ignore_nulls=True is retained inside, since prefix is legitimately optional.

🤖 Generated with Claude Code

…ix codes

get_entry built codes with concat_str(..., ignore_nulls=True), so a null
code column collapsed to just the prefix (e.g. a null lab_category became
the code 'LAB-RES'), which then survived drop_nulls and entered the
vocabulary as a meaningless token. Wrap the concat in
when(col(code).is_not_null()) so such rows become null and are removed by
the existing drop_nulls; ignore_nulls is kept for the optional prefix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@brettbj

brettbj commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

This does seem like an issue but I'm not 100% sure the proposed fix is the right way to handle either

@burkh4rt

Copy link
Copy Markdown
Contributor

I definitey agree that we should have a principled way to handle this

@burkh4rt

Copy link
Copy Markdown
Contributor

This seems like a reasonable solution

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an event-collation bug where rows with a null category could still produce a non-null code (the bare prefix) due to concat_str(..., ignore_nulls=True), causing those rows to incorrectly survive drop_nulls and enter the vocabulary.

Changes:

  • Make code evaluate to null when the source code/category column is null by guarding the concatenation with pl.when(pl.col(code).is_not_null()).then(...).
  • Preserve the existing behavior that prefix is optional by retaining ignore_nulls=True inside the concatenation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@burkh4rt
burkh4rt merged commit 85622d3 into master Jun 10, 2026
1 check passed
@burkh4rt
burkh4rt deleted the fix/null-category-codes branch June 10, 2026 16:40
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.

3 participants