Skip to content

Document that silhouette_samples ignores kwargs for metric="precomputed" - #719

Open
ApparentlyTejas wants to merge 1 commit into
tslearn-team:mainfrom
ApparentlyTejas:fix/silhouette-samples-precomputed-docs
Open

Document that silhouette_samples ignores kwargs for metric="precomputed"#719
ApparentlyTejas wants to merge 1 commit into
tslearn-team:mainfrom
ApparentlyTejas:fix/silhouette-samples-precomputed-docs

Conversation

@ApparentlyTejas

Copy link
Copy Markdown

Summary

silhouette_samples(..., metric="precomputed") returns before metric_params, n_jobs, verbose, or **kwds are ever consulted, so they're silently ignored — no distance function is called on a precomputed matrix, matching sklearn's own silhouette_samples. That behavior wasn't documented anywhere on the function itself.

This PR documents it rather than raising, for parity with sklearn and with silhouette_score's existing precomputed handling (see discussion on #713).

  • Docstring notes on metric_params, n_jobs, verbose, and **kwds stating they're ignored when metric="precomputed".
  • Inline comment at the early-return explaining why.
  • Regression test asserting metric_params/n_jobs/verbose passed alongside metric="precomputed" don't change the result.

Closes #714.

Test plan

  • Manually traced the precomputed branch against the docstring changes and the existing test comments at tests/test_clustering.py:335-337, which already assumed this behavior.
  • Added test_silhouette_samples assertion comparing a baseline precomputed call against one with metric_params/n_jobs/verbose set, asserting equal output.
  • Couldn't run the suite locally due to an unrelated broken Python/libexpat install in my environment — would appreciate CI running it.

metric_params, n_jobs, verbose and **kwds have no effect when
metric="precomputed" since no distance function is called in that
branch, mirroring sklearn's own silhouette_samples. This was already
true and already covered implicitly by test comments, but undocumented
on the function itself. Add explicit docstring notes, an inline
comment at the early-return, and a regression test locking in that
these arguments are a no-op and don't change the result.

Closes tslearn-team#714
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.21%. Comparing base (a4eb51a) to head (2d81d3c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #719   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files          83       83           
  Lines        7902     7905    +3     
=======================================
+ Hits         7524     7527    +3     
  Misses        378      378           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

silhouette_samples(metric='precomputed') silently ignores metric_params, n_jobs, verbose

1 participant