Skip to content

Handle falsy explicit impute replacements#1087

Open
driavysinus wants to merge 4 commits into
theislab:mainfrom
driavysinus:fix/explicit-impute-falsy-mapping-values
Open

Handle falsy explicit impute replacements#1087
driavysinus wants to merge 4 commits into
theislab:mainfrom
driavysinus:fix/explicit-impute-falsy-mapping-values

Conversation

@driavysinus

@driavysinus driavysinus commented Jul 8, 2026

Copy link
Copy Markdown

Problem

explicit_impute() accepts mapping replacement values typed as str | int | float, but falsy values such as 0 are skipped because the current implementation checks replacement values by truthiness.

For example, replacement={'intcol': 0} should be a valid per-column replacement, but 0 is treated as if no replacement was configured.

Fix

Use mapping key existence checks in _extract_impute_value() and is not None in the caller, so valid falsy replacements are applied. Added a regression test for replacement={'intcol': 0}.

Validation

.venv\Scripts\pytest.exe tests\preprocessing\test_imputation.py::test_explicit_impute_subset_accepts_falsy_replacement_value -q
# 2 passed

.venv\Scripts\pytest.exe tests\preprocessing\test_imputation.py::test_explicit_impute_subset -q
# 2 passed

python -m py_compile ehrapy\preprocessing\_imputation.py tests\preprocessing\test_imputation.py

Risk

Low. This changes behavior only for mapping replacement values that are valid by type but were previously skipped because they are falsy.

@Zethson
Zethson requested a review from sueoglu July 9, 2026 07:12
@driavysinus
driavysinus marked this pull request as ready for review July 9, 2026 12:38
@driavysinus

driavysinus commented Jul 9, 2026

Copy link
Copy Markdown
Author

The GPU tests pass check did not execute the GPU suite; it failed at the fork-permission gate in .github/workflows/test_gpu.yml. The maintainer-applied skip-gpu-ci label reflects that.

@Zethson
Zethson requested a review from eroell July 10, 2026 07:02

@sueoglu sueoglu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Comment thread tests/preprocessing/test_imputation.py Outdated


@pytest.mark.parametrize("array_type", ARRAY_TYPES_NONNUMERIC)
def test_explicit_impute_subset_accepts_falsy_replacement_value(impute_edata, array_type):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be nice to test other falsy values than only 0, what do you think?

@eroell eroell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see the point. Thank you!

Additional regression test is great, with all previous tests still passing: as this is of course correcting the function towards intended behavior, although for quite a special case.

Please add a release note - this is also to give your work visible credit!

@eroell

eroell commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Seems here on the CI some tests fail, which are most likely unrelated and are due to new releases.

For miceforest, the problem is with the upstream package itself:

AnotherSamWilson/miceforest#104

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants