From e0583dd2b959928ed89766d25ca6fc684b757cae Mon Sep 17 00:00:00 2001 From: Tomas Pereira de Vasconcelos Date: Fri, 26 Dec 2025 18:42:27 +0800 Subject: [PATCH 1/6] Improve test_hist.py Signed-off-by: Tomas Pereira de Vasconcelos --- tests/unit/test_hist.py | 188 +++++++++++++++++++++++++++------------- 1 file changed, 130 insertions(+), 58 deletions(-) diff --git a/tests/unit/test_hist.py b/tests/unit/test_hist.py index 0008bbbe..2f5fbd60 100644 --- a/tests/unit/test_hist.py +++ b/tests/unit/test_hist.py @@ -8,89 +8,161 @@ bin_trace_samples, ) -# Example data +# ============================================================== +# --- bin_trace_samples() +# ============================================================== -SAMPLES_IN = [1, 2, 2, 3, 4] -NBINS = 4 -# NOTE: The x values in DENSITIES_OUT correspond to the centers of -# equally spaced bins over the range [1, 4]. -# This can be counterintuitive for count data, as the bins -# do not align with the integer sample values. -DENSITIES_OUT = [(1.375, 1), (2.125, 2), (2.875, 1), (3.625, 1)] -X_OUT, Y_OUT = zip(*DENSITIES_OUT, strict=True) -WEIGHTS = [1, 1, 1, 1, 9] +# --- Basic functionality --- + + +@pytest.mark.parametrize( + ("samples", "nbins", "expected"), + [ + # Basic case with repeated values + ([1, 2, 2, 3, 4], 4, [(1.375, 1), (2.125, 2), (2.875, 1), (3.625, 1)]), + # Single bin aggregates all samples + ([1, 2, 3], 1, [(2.0, 3)]), + # Uniform distribution + ([0, 1, 2, 3], 4, [(0.375, 1), (1.125, 1), (1.875, 1), (2.625, 1)]), + # All identical samples go to rightmost bin + ([3, 3, 3], 2, [(2.75, 0), (3.25, 3)]), + # Negative values + ([-2, -1, 0, 1], 2, [(-1.25, 2), (0.25, 2)]), + ], + ids=["basic", "single_bin", "uniform", "identical", "negative"], +) +def test_basic_binning( + samples: list[float], nbins: int, expected: list[tuple[float, float]] +) -> None: + result = bin_trace_samples(samples, nbins=nbins) + assert result == expected -# ============================================================== -# --- estimate_density_trace() -# ============================================================== + +def test_float_samples_binning() -> None: + result = bin_trace_samples([0.1, 0.5, 0.9], nbins=3) + x_vals, y_vals = zip(*result, strict=True) + assert x_vals == pytest.approx((0.233, 0.5, 0.767), rel=1e-2) + assert y_vals == (1.0, 1.0, 1.0) -def test_bin_trace_samples_simple() -> None: - density_trace = bin_trace_samples(trace_samples=SAMPLES_IN, nbins=NBINS) - x, y = zip(*density_trace, strict=True) - assert x == X_OUT - assert y == Y_OUT +@pytest.mark.parametrize("nbins", [1, 2, 5, 10, 50]) +def test_output_length_matches_nbins(nbins: int) -> None: + result = bin_trace_samples([1, 2, 3, 4, 5], nbins=nbins) + assert len(result) == nbins -@pytest.mark.parametrize("nbins", [2, 5, 8, 11]) -def test_bin_trace_samples_nbins(nbins: int) -> None: - density_trace = bin_trace_samples(trace_samples=SAMPLES_IN, nbins=nbins) - assert len(density_trace) == nbins +@pytest.mark.parametrize( + "input_type", + [list, tuple, np.array], + ids=["list", "tuple", "ndarray"], +) +def test_accepts_various_input_types(input_type: type) -> None: + samples = input_type([1, 2, 3]) + result = bin_trace_samples(samples, nbins=2) + assert len(result) == 2 + assert all(isinstance(x, float) and isinstance(y, float) for x, y in result) -@pytest.mark.parametrize("non_finite_value", [np.inf, np.nan, float("inf"), float("nan")]) -def test_bin_trace_samples_fails_for_non_finite_values(non_finite_value: float) -> None: - err_msg = "The samples array should not contain any infs or NaNs." - with pytest.raises(ValueError, match=err_msg): - bin_trace_samples(trace_samples=[*SAMPLES_IN[:-1], non_finite_value], nbins=NBINS) +def test_counts_sum_to_sample_size() -> None: + samples = list(range(100)) + result = bin_trace_samples(samples, nbins=7) + total_count = sum(y for _, y in result) + assert total_count == len(samples) -def test_bin_trace_samples_weights() -> None: - density_trace = bin_trace_samples( - trace_samples=SAMPLES_IN, - nbins=NBINS, - weights=WEIGHTS, - ) - x, y = zip(*density_trace, strict=True) - assert x == X_OUT - assert np.argmax(y) == len(y) - 1 +def test_bin_centers_within_data_range() -> None: + samples = [10, 20, 30, 40, 50] + result = bin_trace_samples(samples, nbins=5) + centers = [x for x, _ in result] + assert all(min(samples) <= c <= max(samples) for c in centers) -def test_bin_trace_samples_weights_not_same_length() -> None: - with pytest.raises( - ValueError, match="The weights array should have the same length as the samples array" - ): - bin_trace_samples(trace_samples=SAMPLES_IN, nbins=NBINS, weights=[1, 1, 1]) +# --- Weights --- -@pytest.mark.parametrize("non_finite_value", [np.inf, np.nan, float("inf"), float("nan")]) -def test_bin_trace_samples_weights_fails_for_non_finite_values( - non_finite_value: float, +@pytest.mark.parametrize( + ("samples", "weights", "nbins", "expected_counts"), + [ + # Weights shift distribution + ([1, 2, 3], [10, 1, 1], 3, [10, 1, 1]), + # Zero weights effectively exclude samples + ([1, 2, 3], [1, 0, 1], 3, [1, 0, 1]), + # Fractional weights + ([1, 2], [0.5, 1.5], 2, [0.5, 1.5]), + ], + ids=["heavy_first", "zero_weight", "fractional"], +) +def test_weights_affect_counts( + samples: list[float], + weights: list[float], + nbins: int, + expected_counts: list[float], ) -> None: - err_msg = "The weights array should not contain any infs or NaNs." - with pytest.raises(ValueError, match=err_msg): - bin_trace_samples( - trace_samples=SAMPLES_IN, - nbins=NBINS, - weights=[*WEIGHTS[:-1], non_finite_value], - ) + result = bin_trace_samples(samples, nbins=nbins, weights=weights) + counts = [y for _, y in result] + assert counts == pytest.approx(expected_counts) + + +def test_weighted_counts_sum_to_weight_sum() -> None: + samples = [1, 2, 3, 4, 5] + weights = [2.0, 3.0, 1.5, 0.5, 4.0] + result = bin_trace_samples(samples, nbins=3, weights=weights) + assert sum(y for _, y in result) == pytest.approx(sum(weights)) + + +# --- Error handling --- + + +@pytest.mark.parametrize( + "non_finite", + [np.inf, -np.inf, np.nan, float("inf"), float("nan")], + ids=["inf", "neg_inf", "nan", "float_inf", "float_nan"], +) +def test_rejects_non_finite_samples(non_finite: float) -> None: + with pytest.raises(ValueError, match="samples array should not contain any infs or NaNs"): + bin_trace_samples([1, 2, non_finite], nbins=2) + + +@pytest.mark.parametrize( + "non_finite", + [np.inf, -np.inf, np.nan, float("inf"), float("nan")], + ids=["inf", "neg_inf", "nan", "float_inf", "float_nan"], +) +def test_rejects_non_finite_weights(non_finite: float) -> None: + with pytest.raises(ValueError, match="weights array should not contain any infs or NaNs"): + bin_trace_samples([1, 2, 3], nbins=2, weights=[1, non_finite, 1]) + + +@pytest.mark.parametrize( + ("samples", "weights"), + [ + ([1, 2, 3], [1, 2]), + ([1, 2], [1, 2, 3]), + ([1], []), + ], + ids=["weights_short", "weights_long", "empty_weights"], +) +def test_rejects_mismatched_weights_length(samples: list[float], weights: list[float]) -> None: + with pytest.raises(ValueError, match="weights array should have the same length"): + bin_trace_samples(samples, nbins=2, weights=weights) # ============================================================== -# --- estimate_densities() +# --- bin_samples() # ============================================================== def test_bin_samples() -> None: - densities = bin_samples( - samples=[[SAMPLES_IN], [SAMPLES_IN]], - nbins=NBINS, - ) + samples = [1, 2, 2, 3, 4] + nbins = 4 + expected = [(1.375, 1), (2.125, 2), (2.875, 1), (3.625, 1)] + x_out, y_out = zip(*expected, strict=True) + densities = bin_samples(samples=[[samples], [samples]], nbins=nbins) assert len(densities) == 2 for densities_row in densities: assert len(densities_row) == 1 density_trace = next(iter(densities_row)) x, y = zip(*density_trace, strict=True) - assert x == X_OUT - assert y == Y_OUT + assert x == x_out + assert y == y_out From e21e017acb961fabdb40d237b8e04c5305c0eb2a Mon Sep 17 00:00:00 2001 From: Tomas Pereira de Vasconcelos Date: Fri, 24 Jul 2026 19:23:09 +0200 Subject: [PATCH 2/6] Extend bin_samples() coverage, tighten assertions, and add changelog entry --- docs/reference/changelog.md | 1 + tests/unit/test_hist.py | 91 +++++++++++++++++++++++++------------ 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/docs/reference/changelog.md b/docs/reference/changelog.md index e7bf832f..fe2bfd4a 100644 --- a/docs/reference/changelog.md +++ b/docs/reference/changelog.md @@ -7,6 +7,7 @@ Unreleased changes ### CI/CD +- Improve the unit test suite for the `ridgeplot._hist` module ({gh-pr}`365`) - Bump actions/download-artifact from 7 to 8 ({gh-pr}`368`) - Bump actions/upload-artifact from 6 to 7 ({gh-pr}`369`) - Bump sigstore/gh-action-sigstore-python from 3.2.0 to 3.3.0 ({gh-pr}`370`) diff --git a/tests/unit/test_hist.py b/tests/unit/test_hist.py index 2f5fbd60..ea0da41c 100644 --- a/tests/unit/test_hist.py +++ b/tests/unit/test_hist.py @@ -1,5 +1,7 @@ from __future__ import annotations +from typing import TYPE_CHECKING + import numpy as np import pytest @@ -8,6 +10,12 @@ bin_trace_samples, ) +if TYPE_CHECKING: + from collections.abc import Callable + from typing import Any + +NON_FINITE_VALUES = [np.inf, -np.inf, np.nan] + # ============================================================== # --- bin_trace_samples() # ============================================================== @@ -20,12 +28,17 @@ ("samples", "nbins", "expected"), [ # Basic case with repeated values + # NOTE: The expected x values correspond to the centers of + # equally spaced bins over the range [min, max] of the + # samples. This can be counterintuitive for count data, + # as the bins do not align with the integer sample values. ([1, 2, 2, 3, 4], 4, [(1.375, 1), (2.125, 2), (2.875, 1), (3.625, 1)]), # Single bin aggregates all samples ([1, 2, 3], 1, [(2.0, 3)]), # Uniform distribution ([0, 1, 2, 3], 4, [(0.375, 1), (1.125, 1), (1.875, 1), (2.625, 1)]), - # All identical samples go to rightmost bin + # All identical samples fall in the rightmost bin + # (NumPy pads the zero-width range by +/-0.5) ([3, 3, 3], 2, [(2.75, 0), (3.25, 3)]), # Negative values ([-2, -1, 0, 1], 2, [(-1.25, 2), (0.25, 2)]), @@ -42,7 +55,7 @@ def test_basic_binning( def test_float_samples_binning() -> None: result = bin_trace_samples([0.1, 0.5, 0.9], nbins=3) x_vals, y_vals = zip(*result, strict=True) - assert x_vals == pytest.approx((0.233, 0.5, 0.767), rel=1e-2) + assert x_vals == pytest.approx((7 / 30, 0.5, 23 / 30)) assert y_vals == (1.0, 1.0, 1.0) @@ -54,14 +67,16 @@ def test_output_length_matches_nbins(nbins: int) -> None: @pytest.mark.parametrize( "input_type", - [list, tuple, np.array], + [list, tuple, np.asarray], ids=["list", "tuple", "ndarray"], ) -def test_accepts_various_input_types(input_type: type) -> None: - samples = input_type([1, 2, 3]) - result = bin_trace_samples(samples, nbins=2) +def test_accepts_various_input_types(input_type: Callable[[list[int]], Any]) -> None: + result = bin_trace_samples(input_type([1, 2, 3]), nbins=2) assert len(result) == 2 - assert all(isinstance(x, float) and isinstance(y, float) for x, y in result) + # The output should always be normalised to built-in floats + # (note: isinstance() checks wouldn't cut it here since + # np.float64 is also a subclass of the built-in float) + assert all(type(x) is float and type(y) is float for x, y in result) def test_counts_sum_to_sample_size() -> None: @@ -84,7 +99,7 @@ def test_bin_centers_within_data_range() -> None: @pytest.mark.parametrize( ("samples", "weights", "nbins", "expected_counts"), [ - # Weights shift distribution + # Each sample falls in its own bin, so the weights become the counts ([1, 2, 3], [10, 1, 1], 3, [10, 1, 1]), # Zero weights effectively exclude samples ([1, 2, 3], [1, 0, 1], 3, [1, 0, 1]), @@ -114,21 +129,13 @@ def test_weighted_counts_sum_to_weight_sum() -> None: # --- Error handling --- -@pytest.mark.parametrize( - "non_finite", - [np.inf, -np.inf, np.nan, float("inf"), float("nan")], - ids=["inf", "neg_inf", "nan", "float_inf", "float_nan"], -) +@pytest.mark.parametrize("non_finite", NON_FINITE_VALUES) def test_rejects_non_finite_samples(non_finite: float) -> None: with pytest.raises(ValueError, match="samples array should not contain any infs or NaNs"): bin_trace_samples([1, 2, non_finite], nbins=2) -@pytest.mark.parametrize( - "non_finite", - [np.inf, -np.inf, np.nan, float("inf"), float("nan")], - ids=["inf", "neg_inf", "nan", "float_inf", "float_nan"], -) +@pytest.mark.parametrize("non_finite", NON_FINITE_VALUES) def test_rejects_non_finite_weights(non_finite: float) -> None: with pytest.raises(ValueError, match="weights array should not contain any infs or NaNs"): bin_trace_samples([1, 2, 3], nbins=2, weights=[1, non_finite, 1]) @@ -155,14 +162,40 @@ def test_rejects_mismatched_weights_length(samples: list[float], weights: list[f def test_bin_samples() -> None: samples = [1, 2, 2, 3, 4] - nbins = 4 - expected = [(1.375, 1), (2.125, 2), (2.875, 1), (3.625, 1)] - x_out, y_out = zip(*expected, strict=True) - densities = bin_samples(samples=[[samples], [samples]], nbins=nbins) - assert len(densities) == 2 - for densities_row in densities: - assert len(densities_row) == 1 - density_trace = next(iter(densities_row)) - x, y = zip(*density_trace, strict=True) - assert x == x_out - assert y == y_out + expected_trace = [(1.375, 1.0), (2.125, 2.0), (2.875, 1.0), (3.625, 1.0)] + densities = bin_samples(samples=[[samples], [samples]], nbins=4) + assert densities == [[expected_trace], [expected_trace]] + + +def test_bin_samples_preserves_shape() -> None: + densities = bin_samples(samples=[[[0, 1], [2, 3, 4]], [[5, 6, 7, 8]]], nbins=3) + assert [len(row) for row in densities] == [2, 1] + assert all(len(trace) == 3 for row in densities for trace in row) + + +def test_bin_samples_broadcasts_flat_weights() -> None: + """A single flat weights vector should be applied to all traces.""" + trace_a, trace_b = [1, 2, 3], [4, 5, 6] + weights = [1, 2, 3] + densities = bin_samples(samples=[[trace_a, trace_b]], nbins=2, sample_weights=weights) + assert densities == [ + [ + bin_trace_samples(trace_a, nbins=2, weights=weights), + bin_trace_samples(trace_b, nbins=2, weights=weights), + ] + ] + + +def test_bin_samples_per_trace_weights() -> None: + """Per-trace weights (shallow form) should be matched to each trace.""" + trace_a, trace_b = [1, 2, 3], [4, 5, 6, 7] + weights_a = [1, 2, 3] + densities = bin_samples( + samples=[[trace_a], [trace_b]], + nbins=2, + sample_weights=[weights_a, None], + ) + assert densities == [ + [bin_trace_samples(trace_a, nbins=2, weights=weights_a)], + [bin_trace_samples(trace_b, nbins=2)], + ] From f42678ead6ea35a6fe8dd63fe4340522a0c97abe Mon Sep 17 00:00:00 2001 From: Tomas Pereira de Vasconcelos Date: Fri, 24 Jul 2026 19:28:31 +0200 Subject: [PATCH 3/6] Avoid unidiomatic-typecheck pattern flagged by Codacy --- tests/unit/test_hist.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_hist.py b/tests/unit/test_hist.py index ea0da41c..09ebcc75 100644 --- a/tests/unit/test_hist.py +++ b/tests/unit/test_hist.py @@ -76,7 +76,7 @@ def test_accepts_various_input_types(input_type: Callable[[list[int]], Any]) -> # The output should always be normalised to built-in floats # (note: isinstance() checks wouldn't cut it here since # np.float64 is also a subclass of the built-in float) - assert all(type(x) is float and type(y) is float for x, y in result) + assert {type(value) for xy_pair in result for value in xy_pair} == {float} def test_counts_sum_to_sample_size() -> None: From 0a8cbba0cb170b46b0c2089357eee26dede8c400 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:53:18 +0000 Subject: [PATCH 4/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/unit/test_hist.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/test_hist.py b/tests/unit/test_hist.py index 6426fda2..dc3ffebb 100644 --- a/tests/unit/test_hist.py +++ b/tests/unit/test_hist.py @@ -63,6 +63,8 @@ def test_float_samples_binning() -> None: def test_output_length_matches_nbins(nbins: int) -> None: result = bin_trace_samples([1, 2, 3, 4, 5], nbins=nbins) assert len(result) == nbins + + @pytest.mark.parametrize( "non_finite_value", [np.inf, np.nan, float("inf"), float("nan")], From 83a226209523ea6f8b991a3fc55d73d9822506f0 Mon Sep 17 00:00:00 2001 From: Tomas Pereira de Vasconcelos Date: Sat, 25 Jul 2026 06:10:20 +0200 Subject: [PATCH 5/6] Remove stale tests reintroduced by merge conflict resolution --- tests/unit/test_hist.py | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/tests/unit/test_hist.py b/tests/unit/test_hist.py index dc3ffebb..09ebcc75 100644 --- a/tests/unit/test_hist.py +++ b/tests/unit/test_hist.py @@ -65,17 +65,6 @@ def test_output_length_matches_nbins(nbins: int) -> None: assert len(result) == nbins -@pytest.mark.parametrize( - "non_finite_value", - [np.inf, np.nan, float("inf"), float("nan")], - ids=["np-inf", "np-nan", "float-inf", "float-nan"], -) -def test_bin_trace_samples_fails_for_non_finite_values(non_finite_value: float) -> None: - err_msg = "The samples array should not contain any infs or NaNs." - with pytest.raises(ValueError, match=err_msg): - bin_trace_samples(trace_samples=[*SAMPLES_IN[:-1], non_finite_value], nbins=NBINS) - - @pytest.mark.parametrize( "input_type", [list, tuple, np.asarray], @@ -166,23 +155,6 @@ def test_rejects_mismatched_weights_length(samples: list[float], weights: list[f bin_trace_samples(samples, nbins=2, weights=weights) -@pytest.mark.parametrize( - "non_finite_value", - [np.inf, np.nan, float("inf"), float("nan")], - ids=["np-inf", "np-nan", "float-inf", "float-nan"], -) -def test_bin_trace_samples_weights_fails_for_non_finite_values( - non_finite_value: float, -) -> None: - err_msg = "The weights array should not contain any infs or NaNs." - with pytest.raises(ValueError, match=err_msg): - bin_trace_samples( - trace_samples=SAMPLES_IN, - nbins=NBINS, - weights=[*WEIGHTS[:-1], non_finite_value], - ) - - # ============================================================== # --- bin_samples() # ============================================================== From 9845633d1fad85034d03e3ce5ed77cd251d5cd42 Mon Sep 17 00:00:00 2001 From: Tomas Pereira de Vasconcelos Date: Tue, 28 Jul 2026 12:41:03 +0200 Subject: [PATCH 6/6] Add clear validation errors for empty samples and invalid sample weights --- docs/reference/changelog.md | 7 ++++ src/ridgeplot/_hist.py | 14 ++----- src/ridgeplot/_kde.py | 54 +++++++++++++++++++------- src/ridgeplot/_ridgeplot.py | 17 +++++++-- src/ridgeplot/_utils.py | 15 +++++++- tests/unit/test_hist.py | 74 +++++++++++++++++++++++++++++++++++- tests/unit/test_kde.py | 33 ++++++++++++++++ tests/unit/test_ridgeplot.py | 27 +++++++++++++ tests/unit/test_utils.py | 39 ++++++++++++++++++- 9 files changed, 247 insertions(+), 33 deletions(-) diff --git a/docs/reference/changelog.md b/docs/reference/changelog.md index 53adb317..7e018264 100644 --- a/docs/reference/changelog.md +++ b/docs/reference/changelog.md @@ -9,9 +9,16 @@ Unreleased changes - Implement a new `template` parameter to allow users to specify a Plotly figure template ({gh-pr}`389`) +### Bug fixes + +- Raise a clear `ValueError` when an empty samples array is passed to either density estimation method (KDE or histogram binning), instead of failing downstream with obscure errors ({gh-pr}`365`) +- Raise clear `ValueError`s for invalid `sample_weights` values (negative or all-zero weights), instead of failing downstream with obscure errors ({gh-pr}`365`) +- Improve the error messages raised when the length of a `sample_weights` array does not match the length of the corresponding samples array, or when the number of rows in a shallow per-row attributes array (e.g., `sample_weights` or `labels`) does not match the number of rows in the samples array ({gh-pr}`365`) + ### Documentation - Add a section on theming with Plotly templates to the getting started guide ({gh-pr}`389`) +- Fix the description of the `sample_weights` parameter in the `ridgeplot()` docstring, which incorrectly stated that the weights only applied to KDE, and document the accepted forms and their broadcasting semantics ({gh-pr}`365`) ### CI/CD diff --git a/src/ridgeplot/_hist.py b/src/ridgeplot/_hist.py index 4f932ba7..4a1d3e87 100644 --- a/src/ridgeplot/_hist.py +++ b/src/ridgeplot/_hist.py @@ -6,7 +6,7 @@ import numpy as np -from ridgeplot._kde import normalize_sample_weights +from ridgeplot._kde import normalize_sample_weights, validate_trace_samples_and_weights if TYPE_CHECKING: from ridgeplot._types import ( @@ -25,16 +25,8 @@ def bin_trace_samples( nbins: int, weights: SampleWeights = None, ) -> DensityTrace: - trace_samples = np.asarray(trace_samples, dtype=float) - if not np.isfinite(trace_samples).all(): - raise ValueError("The samples array should not contain any infs or NaNs.") - if weights is not None: - weights = np.asarray(weights, dtype=float) - if len(weights) != len(trace_samples): - raise ValueError("The weights array should have the same length as the samples array.") - if not np.isfinite(weights).all(): - raise ValueError("The weights array should not contain any infs or NaNs.") - hist_counts, hist_edges = np.histogram(trace_samples, bins=nbins, weights=weights) + samples_array, weights_array = validate_trace_samples_and_weights(trace_samples, weights) + hist_counts, hist_edges = np.histogram(samples_array, bins=nbins, weights=weights_array) bin_centers = 0.5 * (hist_edges[:-1] + hist_edges[1:]) return [(float(x), float(y)) for x, y in zip(bin_centers, hist_counts, strict=True)] diff --git a/src/ridgeplot/_kde.py b/src/ridgeplot/_kde.py index bc851bda..22c3b2ee 100644 --- a/src/ridgeplot/_kde.py +++ b/src/ridgeplot/_kde.py @@ -98,6 +98,40 @@ def normalize_sample_weights( return sample_weights +def validate_trace_samples_and_weights( + trace_samples: SamplesTrace, + weights: SampleWeights, +) -> tuple[npt.NDArray[np.floating[Any]], npt.NDArray[np.floating[Any]] | None]: + """Validate a trace's samples and weights and coerce them to NumPy arrays. + + This validation logic is shared between the KDE + (:func:`estimate_density_trace`) and histogram binning + (:func:`ridgeplot._hist.bin_trace_samples`) paths. + """ + samples_array = np.asarray(trace_samples, dtype=float) + if len(samples_array) == 0: + raise ValueError("The samples array should not be empty.") + if not np.isfinite(samples_array).all(): + raise ValueError("The samples array should not contain any infs or NaNs.") + if weights is None: + return samples_array, None + weights_array = np.asarray(weights, dtype=float) + if len(weights_array) != len(samples_array): + raise ValueError( + f"The weights array should have the same length as the samples array " + f"(got {len(weights_array)} weights for {len(samples_array)} samples). " + f"Note that a single flat array of weights is broadcast to all traces " + f"in the samples array." + ) + if not np.isfinite(weights_array).all(): + raise ValueError("The weights array should not contain any infs or NaNs.") + if np.any(weights_array < 0): + raise ValueError("The weights array should not contain negative values.") + if not np.any(weights_array): + raise ValueError("The weights array should not be all zeros.") + return samples_array, weights_array + + def estimate_density_trace( trace_samples: SamplesTrace, points: KDEPoints, @@ -110,16 +144,14 @@ def estimate_density_trace( For a given set of sample values, computes the kernel densities (KDE) at the given points. """ - trace_samples = np.asarray(trace_samples, dtype=float) - if not np.isfinite(trace_samples).all(): - raise ValueError("The samples array should not contain any infs or NaNs.") + samples_array, weights_array = validate_trace_samples_and_weights(trace_samples, weights) if isinstance(points, int): # By default, we'll use a 'hard' KDE span. That is, we'll # evaluate the densities and N equally spaced points # over the range [min(samples), max(samples)] density_x = np.linspace( - start=min(trace_samples), - stop=max(trace_samples), + start=min(samples_array), + stop=max(samples_array), num=points, ) else: @@ -130,15 +162,9 @@ def estimate_density_trace( f"The 'points' at which KDE is computed should be represented by a " f"one-dimensional array, got an array of shape {density_x.shape} instead." ) - if weights is not None: - weights = np.asarray(weights, dtype=float) - if len(weights) != len(trace_samples): - raise ValueError("The weights array should have the same length as the samples array.") - if not np.isfinite(weights).all(): - raise ValueError("The weights array should not contain any infs or NaNs.") # ref: https://github.com/tpvasconcelos/ridgeplot/issues/116 - dens = sm.nonparametric.KDEUnivariate(trace_samples) + dens = sm.nonparametric.KDEUnivariate(samples_array) # I'm hard-coding `fft=kernel == "gau" and weights is not None` # to avoid exposing yet another KDE parameter in ridgeplot() @@ -150,9 +176,9 @@ def estimate_density_trace( # directly to the ridgeplot() figure factory. dens.fit( kernel=kernel, - fft=kernel == "gau" and weights is None, + fft=kernel == "gau" and weights_array is None, bw=bandwidth, # pyright: ignore[reportArgumentType] - weights=weights, + weights=weights_array, ) density_y = dens.evaluate(density_x) density_y = _validate_densities(x=density_x, y=density_y, kernel=kernel) diff --git a/src/ridgeplot/_ridgeplot.py b/src/ridgeplot/_ridgeplot.py index 6bbbc185..a1f3df80 100644 --- a/src/ridgeplot/_ridgeplot.py +++ b/src/ridgeplot/_ridgeplot.py @@ -266,9 +266,20 @@ def ridgeplot( .. versionadded:: 0.3.0 sample_weights : SampleWeightsArray or ShallowSampleWeightsArray or SampleWeights or None - An (optional) array of KDE weights corresponding to each sample. The - weights should have the same shape as the samples array. If not - specified (default), all samples will be weighted equally. + An (optional) array of weights corresponding to each sample. The + weights are used by both density estimation methods (i.e., KDE and + histogram binning). If not specified (default), all samples will be + weighted equally. The following forms are accepted: + + - A single flat array of weights, which will be broadcast to all + traces in the :paramref:`.samples` array. This requires all traces + to have the same number of samples as there are weights. + - A shallow array of weights with one entry per row, where each entry + is either a flat array of weights or ``None``. Each entry will be + applied to all traces in the corresponding row. + - A full (nested) array of weights with the same shape as the + :paramref:`.samples` array, specifying the weights for each trace + individually. norm : NormalisationOption or None The normalisation option to use when normalising the densities. If not diff --git a/src/ridgeplot/_utils.py b/src/ridgeplot/_utils.py index 7a8abc2d..f50e0225 100644 --- a/src/ridgeplot/_utils.py +++ b/src/ridgeplot/_utils.py @@ -232,14 +232,19 @@ def normalise_row_attrs( Raises ------ ValueError - If the number of traces does not match the number of attributes for a - row. + If the number of rows in ``attrs`` does not match the number of rows + in ``l2_target``, or if the number of traces does not match the number + of attributes for a row. Examples -------- >>> densities = [[[(0, 0), (1, 1), (2, 0)]]] # Single row, single trace >>> normalise_row_attrs(["A"], densities) [['A']] + >>> normalise_row_attrs([["A"], ["B"]], densities) + Traceback (most recent call last): + ... + ValueError: Mismatch between number of rows in attrs (2) and samples/densities (1). >>> normalise_row_attrs([["A", "B"]], densities) Traceback (most recent call last): ... @@ -311,6 +316,12 @@ def normalise_row_attrs( if not is_collection_l2(attrs): attrs = [attrs] if len(l2_target) == 1 else [[attr] for attr in attrs] + if len(attrs) != len(l2_target): + raise ValueError( + f"Mismatch between number of rows in attrs ({len(attrs)}) " + f"and samples/densities ({len(l2_target)})." + ) + result: list[list[_V]] = [] for i, (row_attrs, row_traces) in enumerate(zip(attrs, l2_target, strict=True)): n_traces = len(row_traces) diff --git a/tests/unit/test_hist.py b/tests/unit/test_hist.py index 09ebcc75..009930e7 100644 --- a/tests/unit/test_hist.py +++ b/tests/unit/test_hist.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re from typing import TYPE_CHECKING import numpy as np @@ -87,12 +88,26 @@ def test_counts_sum_to_sample_size() -> None: def test_bin_centers_within_data_range() -> None: + # NOTE: This property does not hold for the degenerate case where all + # samples are identical, since NumPy pads the zero-width range by + # +/-0.5 and a bin center can then fall outside the data range + # (e.g., [3, 3, 3] with nbins=2 produces a center at 2.75 < 3). samples = [10, 20, 30, 40, 50] result = bin_trace_samples(samples, nbins=5) centers = [x for x, _ in result] assert all(min(samples) <= c <= max(samples) for c in centers) +def test_single_sample_falls_in_middle_bin() -> None: + """A single sample gets NumPy's +/-0.5 range padding, placing all of the + mass in the middle bin (unlike the all-identical case, where the mass + falls in the rightmost bin).""" + result = bin_trace_samples([5], nbins=3) + x_vals, y_vals = zip(*result, strict=True) + assert x_vals == pytest.approx((14 / 3, 5.0, 16 / 3)) + assert y_vals == (0.0, 1.0, 0.0) + + # --- Weights --- @@ -129,6 +144,11 @@ def test_weighted_counts_sum_to_weight_sum() -> None: # --- Error handling --- +def test_rejects_empty_samples() -> None: + with pytest.raises(ValueError, match="samples array should not be empty"): + bin_trace_samples([], nbins=3) + + @pytest.mark.parametrize("non_finite", NON_FINITE_VALUES) def test_rejects_non_finite_samples(non_finite: float) -> None: with pytest.raises(ValueError, match="samples array should not contain any infs or NaNs"): @@ -155,6 +175,16 @@ def test_rejects_mismatched_weights_length(samples: list[float], weights: list[f bin_trace_samples(samples, nbins=2, weights=weights) +def test_rejects_negative_weights() -> None: + with pytest.raises(ValueError, match="weights array should not contain negative values"): + bin_trace_samples([1, 2, 3], nbins=2, weights=[-1, 1, 1]) + + +def test_rejects_all_zero_weights() -> None: + with pytest.raises(ValueError, match="weights array should not be all zeros"): + bin_trace_samples([1, 2, 3], nbins=2, weights=[0, 0, 0]) + + # ============================================================== # --- bin_samples() # ============================================================== @@ -186,8 +216,8 @@ def test_bin_samples_broadcasts_flat_weights() -> None: ] -def test_bin_samples_per_trace_weights() -> None: - """Per-trace weights (shallow form) should be matched to each trace.""" +def test_bin_samples_per_row_weights() -> None: + """Shallow weights (one entry per row) should be matched to each row.""" trace_a, trace_b = [1, 2, 3], [4, 5, 6, 7] weights_a = [1, 2, 3] densities = bin_samples( @@ -199,3 +229,43 @@ def test_bin_samples_per_trace_weights() -> None: [bin_trace_samples(trace_a, nbins=2, weights=weights_a)], [bin_trace_samples(trace_b, nbins=2)], ] + + +def test_bin_samples_per_row_weights_broadcast_to_all_traces_in_row() -> None: + """Shallow weights are per-row, *not* per-trace: each entry should be + broadcast to all traces in the corresponding row.""" + trace_a, trace_b, trace_c = [1, 2, 3], [4, 5, 6], [7, 8, 9, 10] + weights_row_1 = [1, 2, 3] + densities = bin_samples( + samples=[[trace_a, trace_b], [trace_c]], + nbins=2, + sample_weights=[weights_row_1, None], + ) + assert densities == [ + [ + bin_trace_samples(trace_a, nbins=2, weights=weights_row_1), + bin_trace_samples(trace_b, nbins=2, weights=weights_row_1), + ], + [bin_trace_samples(trace_c, nbins=2)], + ] + + +def test_bin_samples_rejects_shallow_weights_row_count_mismatch() -> None: + """Shallow weights should have exactly one entry per row.""" + with pytest.raises( + ValueError, + match=re.escape("Mismatch between number of rows in attrs (1) and samples/densities (2)."), + ): + bin_samples(samples=[[[1, 2]], [[3, 4]]], nbins=2, sample_weights=[[1, 2]]) + + +def test_bin_samples_flat_weights_fail_for_ragged_traces() -> None: + """A flat weights vector is broadcast to all traces, so it should fail + (with a helpful error message) when traces have different lengths.""" + err_msg = ( + "The weights array should have the same length as the samples array " + "(got 3 weights for 4 samples). Note that a single flat array of " + "weights is broadcast to all traces in the samples array." + ) + with pytest.raises(ValueError, match=re.escape(err_msg)): + bin_samples(samples=[[[1, 2, 3], [4, 5, 6, 7]]], nbins=2, sample_weights=[1, 2, 3]) diff --git a/tests/unit/test_kde.py b/tests/unit/test_kde.py index a2fe0ba8..ed147db2 100644 --- a/tests/unit/test_kde.py +++ b/tests/unit/test_kde.py @@ -49,6 +49,16 @@ def test_estimate_density_trace_points() -> None: assert np.argmin(y) == 0 +def test_estimate_density_trace_fails_for_empty_samples() -> None: + with pytest.raises(ValueError, match=re.escape("The samples array should not be empty.")): + estimate_density_trace( + trace_samples=[], + points=7, + kernel="gau", + bandwidth="normal_reference", + ) + + @pytest.mark.parametrize( "non_finite_value", [np.inf, np.nan, float("inf"), float("nan")], @@ -124,6 +134,29 @@ def test_estimate_density_trace_weights_fails_for_non_finite_values( ) +def test_estimate_density_trace_fails_for_negative_weights() -> None: + err_msg = "The weights array should not contain negative values." + with pytest.raises(ValueError, match=re.escape(err_msg)): + estimate_density_trace( + trace_samples=[0, 1, 2], + points=7, + kernel="gau", + bandwidth="normal_reference", + weights=[-1, 1, 1], + ) + + +def test_estimate_density_trace_fails_for_all_zero_weights() -> None: + with pytest.raises(ValueError, match=re.escape("The weights array should not be all zeros.")): + estimate_density_trace( + trace_samples=[0, 1, 2], + points=7, + kernel="gau", + bandwidth="normal_reference", + weights=[0, 0, 0], + ) + + # ============================================================== # --- _validate_densities() # ============================================================== diff --git a/tests/unit/test_ridgeplot.py b/tests/unit/test_ridgeplot.py index e9f587a1..b6571e31 100644 --- a/tests/unit/test_ridgeplot.py +++ b/tests/unit/test_ridgeplot.py @@ -151,6 +151,33 @@ def test_nbins() -> None: assert fig.data[0]._plotly_name == "bar" +# ============================================================== +# --- param: sample_weights +# ============================================================== + + +@pytest.mark.parametrize("nbins", [None, 3], ids=["kde", "hist"]) +def test_empty_samples(nbins: int | None) -> None: + with pytest.raises(ValueError, match=re.escape("The samples array should not be empty.")): + ridgeplot(samples=[[[]]], nbins=nbins) + + +@pytest.mark.parametrize("nbins", [None, 3], ids=["kde", "hist"]) +@pytest.mark.parametrize( + ("sample_weights", "err_msg"), + [ + ([-1, 1, 1], "The weights array should not contain negative values."), + ([0, 0, 0], "The weights array should not be all zeros."), + ], + ids=["negative", "all_zeros"], +) +def test_invalid_sample_weights( + nbins: int | None, sample_weights: list[float], err_msg: str +) -> None: + with pytest.raises(ValueError, match=re.escape(err_msg)): + ridgeplot(samples=[[1, 2, 3]], nbins=nbins, sample_weights=sample_weights) + + # ============================================================== # --- param: colorscale # ============================================================== diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 33092628..f61a5072 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -7,7 +7,7 @@ import numpy as np import pytest -from ridgeplot._utils import get_xy_extrema, normalise_min_max +from ridgeplot._utils import get_xy_extrema, normalise_min_max, normalise_row_attrs if TYPE_CHECKING: from collections.abc import Callable @@ -92,6 +92,43 @@ def test_expected_output( assert get_xy_extrema(densities) == expected +class TestNormaliseRowAttrs: + """Tests for the :func:`ridgeplot._utils.normalise_row_attrs` function.""" + + def test_raises_for_row_count_mismatch(self) -> None: + """A mismatch between the number of rows in the attrs and target + arrays should raise a clear error (instead of leaking a bare + ``zip(..., strict=True)`` error).""" + densities = [ + [[(0, 0), (1, 1), (2, 0)]], # Row 1 + [[(1, 0), (2, 1), (3, 0)]], # Row 2 + ] + with pytest.raises( + ValueError, + match=re.escape( + "Mismatch between number of rows in attrs (1) and samples/densities (2)." + ), + ): + normalise_row_attrs([["A"]], densities) + + def test_raises_for_trace_count_mismatch(self) -> None: + densities = [[[(0, 0), (1, 1), (2, 0)]]] # Single row, single trace + with pytest.raises( + ValueError, + match=re.escape("Mismatch between number of traces (1) and number of attrs (2)"), + ): + normalise_row_attrs([["A", "B"]], densities) + + def test_broadcasts_single_attr_to_all_traces_in_row(self) -> None: + densities = [ + [ + [(0, 0), (1, 1), (2, 0)], # Trace 1 + [(1, 0), (2, 1), (3, 0)], # Trace 2 + ], + ] + assert normalise_row_attrs([["A"]], densities) == [["A", "A"]] + + class TestNormaliseMinMax: """Tests for the :func:`ridgeplot._utils.normalise_min_max` function."""