From a44fb3ba98d29b5e437d7343ad09b2ef322b5215 Mon Sep 17 00:00:00 2001 From: David Nicholson Date: Fri, 13 Mar 2026 13:54:23 -0400 Subject: [PATCH 1/5] FIX: bug in transforms.frame_labels.postprocess, fix #821 Call `np.copy` on `frame_labels` first thing, so that we don't indirectly mutate any tensor that shares memory with this numpy array. --- src/vak/transforms/frame_labels/functional.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/vak/transforms/frame_labels/functional.py b/src/vak/transforms/frame_labels/functional.py index e3b89bca5..0c05a68dd 100644 --- a/src/vak/transforms/frame_labels/functional.py +++ b/src/vak/transforms/frame_labels/functional.py @@ -556,6 +556,15 @@ def postprocess( frame_labels : numpy.ndarray Vector of frame labels after post-processing is applied. """ + # NOTE: we need to clone to avoid mutating tensors inside a model, + # since calling `torch.tensor.numpy()` inside the model gives us + # a numpy array that shares memory with the tensor. + # Not doing so can make metrics wrong, + # e.g., for frame labels *before* post-processing, + # if we mutate that tensor here. + # see:https://github.com/vocalpy/vak/issues/821 + frame_labels = np.copy(frame_labels) + frame_labels = row_or_1d(frame_labels) if boundary_labels is not None: boundary_labels = row_or_1d(boundary_labels) From 9b38f41f07785029ca7c749bbd88e02e4813765c Mon Sep 17 00:00:00 2001 From: David Nicholson Date: Fri, 13 Mar 2026 13:56:45 -0400 Subject: [PATCH 2/5] CLN: Unused imports in test_eval.test_frame_classification --- tests/test_eval/test_frame_classification.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_eval/test_frame_classification.py b/tests/test_eval/test_frame_classification.py index 8ac65539a..7ef4e5e77 100644 --- a/tests/test_eval/test_frame_classification.py +++ b/tests/test_eval/test_frame_classification.py @@ -2,8 +2,6 @@ import pytest import vak.config -import vak.common.constants -import vak.common.paths import vak.eval.frame_classification From 512cc59cde4c87695e15ad5103fbe8b315ffe65a Mon Sep 17 00:00:00 2001 From: David Nicholson Date: Fri, 13 Mar 2026 14:58:35 -0400 Subject: [PATCH 3/5] CLN: Make `eval.frame_classification_model` return metrics Calling this a "cleaning" commit because it's not really an enhancement. This just gives us a way to get back the metrics returned by the LightningModule, to avoid a bunch of extra work, like loading them from the csv that gets saved. It lets me add a regression test for https://github.com/vocalpy/vak/issues/821. There is of course a risk of someone relying on this "feature", but long-term I will probably bury this function inside the `cli` package, so that will hopefully be less likely. --- src/vak/eval/frame_classification.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/vak/eval/frame_classification.py b/src/vak/eval/frame_classification.py index 665a0bb4f..f14eae17e 100644 --- a/src/vak/eval/frame_classification.py +++ b/src/vak/eval/frame_classification.py @@ -1,5 +1,4 @@ """Function that evaluates trained models in the frame classification family.""" - from __future__ import annotations import json @@ -30,7 +29,7 @@ def eval_frame_classification_model( num_workers: int, frames_standardizer_path: str | pathlib.Path = None, post_tfm_kwargs: dict | None = None, -) -> None: +) -> dict[str, torch.Tensor]: """Evaluate a trained model. Parameters @@ -215,10 +214,11 @@ def eval_frame_classification_model( logger=trainer_logger, ) # TODO: check for hasattr(model, test_step) and if so run test + # below, [0] because validate returns list of dicts, length of no. of val loaders metric_vals = trainer.validate(model, dataloaders=val_loader)[0] - metric_vals = {f"avg_{k}": v for k, v in metric_vals.items()} - for metric_name, metric_val in metric_vals.items(): + metric_vals_for_df = {f"avg_{k}": v for k, v in metric_vals.items()} + for metric_name, metric_val in metric_vals_for_df.items(): if metric_name.startswith("avg_"): logger.info(f"{metric_name}: {metric_val:0.5f}") @@ -237,7 +237,7 @@ def eval_frame_classification_model( # order metrics by name to be extra sure they will be consistent across runs row.update( sorted( - [(k, v) for k, v in metric_vals.items() if k.startswith("avg_")] + [(k, v) for k, v in metric_vals_for_df.items() if k.startswith("avg_")] ) ) @@ -249,3 +249,6 @@ def eval_frame_classification_model( eval_df.to_csv( eval_csv_path, index=False ) # index is False to avoid having "Unnamed: 0" column when loading + + return metric_vals + From dabf1e0ca82307f8b068795bfd0a93edf0a6c852 Mon Sep 17 00:00:00 2001 From: David Nicholson Date: Fri, 13 Mar 2026 15:02:19 -0400 Subject: [PATCH 4/5] TST: Add regression test for #821 Adds assertions inside `tests/test_eval/test_frame_classification.py::test_eval_frame_classification` that check that metric values are not equal after we apply post-processing transforms to frame labels. See comment added above assertions: not sure if this is the best place to test this long term, but it will work to get a quick fix out. --- tests/test_eval/test_frame_classification.py | 37 ++++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/tests/test_eval/test_frame_classification.py b/tests/test_eval/test_frame_classification.py index 7ef4e5e77..a108c87c8 100644 --- a/tests/test_eval/test_frame_classification.py +++ b/tests/test_eval/test_frame_classification.py @@ -20,9 +20,13 @@ def assert_eval_output_matches_expected(model_name, output_dir): # use ToLabelsWithPostprocessing with *just* majority_vote {'majority_vote': True, 'min_segment_dur': None}, # use ToLabelsWithPostprocessing with *just* min_segment_dur - {'majority_vote': False, 'min_segment_dur': 0.002}, + # NOTE the value of `min_segment_dur=0.05` will make metrics after post-processing *worse* + # but we do that to make sure that this parameter **alone** is having some effect. + # For our current test data, the "good" value of 0.01 we used can in some cases + # have no effect; we want to be sure that's not due to some bug + {'majority_vote': False, 'min_segment_dur': 0.05}, # use ToLabelsWithPostprocessing with majority_vote *and* min_segment_dur - {'majority_vote': True, 'min_segment_dur': 0.002}, + {'majority_vote': True, 'min_segment_dur': 0.05}, ] @@ -67,7 +71,7 @@ def test_eval_frame_classification_model( ) cfg = vak.config.Config.from_toml_path(toml_path) - vak.eval.frame_classification.eval_frame_classification_model( + metric_vals: dict = vak.eval.frame_classification.eval_frame_classification_model( model_config=cfg.eval.model.asdict(), dataset_config=cfg.eval.dataset.asdict(), trainer_config=cfg.eval.trainer.asdict(), @@ -81,6 +85,33 @@ def test_eval_frame_classification_model( assert_eval_output_matches_expected(cfg.eval.model.name, output_dir) + # REGRESSION TEST: these assertions test that + # we fixed https://github.com/vocalpy/vak/issues/821 + # Long-term this may not be the right place to test this, + # but it will work to get out a quick fix; + # NOTE also this is fragile -- I expect the transforms to change the metrics + # for this exact data, but of course you could have a case where + # neither `majority_vote` nor `min_segment_dur` have any effect + # even when `majority_vote=True` and `min_segment_dur > 0.0 + if post_tfm_kwargs is not None: + if post_tfm_kwargs["majority_vote"] is True or ( + post_tfm_kwargs["min_segment_dur"] is not None and post_tfm_kwargs["min_segment_dur"] > 0.0 + ): + for metric_name in ( + "acc", "levenshtein", "character_error_rate", + ): + wout_tfm = metric_vals[f"val_{metric_name}_epoch"] + with_tfm = metric_vals[f"val_{metric_name}_tfm_epoch"] + assert wout_tfm != with_tfm + # for IR metrics, sometimes recall is unchanged + # we mainly want to be sure they are not *all* literally the sma value + assert not all( + [ + metric_vals[f"val_{ir_metric_name}_epoch"] == metric_vals[f"val_{ir_metric_name}_tfm_epoch"] + for ir_metric_name in ("precision", "recall", "fscore", "rval") + ] + ) + @pytest.mark.parametrize( 'path_option_to_change', From 9a781e910efdc9611236c9ae2f2ca11417bcf0b3 Mon Sep 17 00:00:00 2001 From: David Nicholson Date: Fri, 13 Mar 2026 15:52:57 -0400 Subject: [PATCH 5/5] TST/CLN: Rename assert function for clarity Rename `assert_eval_output_is_expected` -> `assert_eval_saves_one_csv`, used in test_eval/test_frame_classification and a couple of other places. --- tests/test_eval/test_frame_classification.py | 4 ++-- tests/test_eval/test_parametric_umap.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_eval/test_frame_classification.py b/tests/test_eval/test_frame_classification.py index a108c87c8..3c58cade9 100644 --- a/tests/test_eval/test_frame_classification.py +++ b/tests/test_eval/test_frame_classification.py @@ -6,7 +6,7 @@ # written as separate function so we can re-use in tests/unit/test_cli/test_eval.py -def assert_eval_output_matches_expected(model_name, output_dir): +def assert_eval_saves_one_csv(model_name, output_dir): eval_csv = sorted(output_dir.glob(f"eval_{model_name}*csv")) assert len(eval_csv) == 1 @@ -83,7 +83,7 @@ def test_eval_frame_classification_model( post_tfm_kwargs=post_tfm_kwargs, ) - assert_eval_output_matches_expected(cfg.eval.model.name, output_dir) + assert_eval_saves_one_csv(cfg.eval.model.name, output_dir) # REGRESSION TEST: these assertions test that # we fixed https://github.com/vocalpy/vak/issues/821 diff --git a/tests/test_eval/test_parametric_umap.py b/tests/test_eval/test_parametric_umap.py index b1744291d..d37cafa9e 100644 --- a/tests/test_eval/test_parametric_umap.py +++ b/tests/test_eval/test_parametric_umap.py @@ -7,7 +7,7 @@ import vak.eval.frame_classification -def assert_eval_output_matches_expected(model_name, output_dir): +def assert_eval_saves_one_csv(model_name, output_dir): eval_csv = sorted(output_dir.glob(f"eval_{model_name}*csv")) assert len(eval_csv) == 1 @@ -57,7 +57,7 @@ def test_eval_parametric_umap_model( num_workers=cfg.eval.num_workers, ) - assert_eval_output_matches_expected(cfg.eval.model.name, output_dir) + assert_eval_saves_one_csv(cfg.eval.model.name, output_dir) @pytest.mark.parametrize(