Skip to content

fix(estimator): pass distance metric parameters as metric_params to NearestNeighbors#1563

Open
TasfinMahmud wants to merge 2 commits into
py-why:mainfrom
TasfinMahmud:fix-1390-distance-metric-params
Open

fix(estimator): pass distance metric parameters as metric_params to NearestNeighbors#1563
TasfinMahmud wants to merge 2 commits into
py-why:mainfrom
TasfinMahmud:fix-1390-distance-metric-params

Conversation

@TasfinMahmud
Copy link
Copy Markdown

Fixes #1390.

NearestNeighbors expects metric parameters (like the V matrix for Mahalanobis distance) to be passed through the metric_params argument, rather than as top-level kwargs.

This PR passes self.distance_metric_params using the metric_params argument in DistanceMatchingEstimator. A regression test is included to ensure method_params={"V": ...} behaves correctly end-to-end without failing.

…earestNeighbors

Fixes issue py-why#1390. NearestNeighbors expects metric parameters (like the V matrix for Mahalanobis distance) to be passed through the metric_params argument, rather than as top-level kwargs.

Signed-off-by: Tasfin Mahmud <tasfinmahmud1@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🤖 This is an automated response from Repo Assist.

Thanks for contributing this fix, @TasfinMahmud! The metric_params= change is exactly right — passing Mahalanobis parameters via **kwargs at the top level of NearestNeighbors has never worked, and this one-liner correction unblocks the feature.

Context for maintainers: Repo Assist previously submitted PRs #1475, #1508, and #1526 for this same issue (#1390). All three have now accumulated merge conflicts since PR #1465 (which refactored exact_match_cols logic in the same file) was merged. This contributor PR is rebased on current main and cleanly supersedes the stale Repo Assist PRs — I'd recommend closing #1475, #1508, and #1526 in favour of this one.

One minor note: PR #1526's fix included metric_params=self.distance_metric_params if self.distance_metric_params else None (passing None when the dict is empty) while this PR passes the empty dict directly (metric_params={}). Both are equivalent for sklearn, so this is not a blocker.

The fix and test look good. Once CI runs and passes, this should be merge-ready.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@11c9a2c442e519ff2b427bf58679f5a525353f76

Signed-off-by: Amit Sharma <amit_sharma@live.com>
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.

Issue with distance_metric_params for dowhy.causal_estimators.distance_matching_estimator

2 participants