From b53157201f12e799eb3bca631cc78d2997badbb4 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Fri, 21 Aug 2026 10:01:01 +0200 Subject: [PATCH 1/2] Measure what the ids cost, and fix the tool that measures The differential check proves a refactor changed no values by digesting every patch and comparing against a reference. Its digest left out history but not the two lineage ids, and `patch_id` is minted per patch for anything not read from a file -- so every patch differed, and a check which always reports a difference reports nothing. The ids join history, for the same reason and a sharper one: the comparison is against a patch built by other code, and these name where a patch came from rather than what the answer is. 775 calls now compare identical against dev. Benchmarks for the charge itself, at both ends: a tiny patch where it is all there is, an array argument where it is not flat because the array is hashed, and real filtering where it should not be findable. The charge is flat, about 35-56 us to canonicalize a call and digest it. That is invisible next to a pass filter (+3%) and doubles a transpose on a two-by-two patch (+156%), so `patch_provenance` earns its keep and no knob comes out. Writing the promised `new`/`update` paragraph turned up a footgun worth saying out loud: they carry both ids through untouched, because they are how a patch function builds its own result and stamping there would count every operation twice -- so changing data through `new` yourself leaves the ids saying it did not change. --- benchmarks/test_patch_benchmarks.py | 53 ++++++++++++++++++++++++++++ docs/tutorial/patch.qmd | 23 ++++++++++++ scripts/differential_check.py | 10 ++++-- tests/test_workflow/test_identity.py | 30 ++++++++++++++++ 4 files changed, 114 insertions(+), 2 deletions(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 525fb1bbe..3d32e5fc9 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -7,6 +7,7 @@ import pytest import dascore as dc +from dascore.config import config_context from dascore.utils.patch import get_start_stop_step @@ -428,3 +429,55 @@ def test_align_1d_shift_valid(self, patch_2d_with_1d_shift): """Benchmark 2D patch with 1D shift coordinate (300 shifts), valid mode.""" patch = patch_2d_with_1d_shift patch.align_to_coord(time="shift_time", mode="valid") + + +class TestIdentityOverhead: + """ + What maintaining the lineage ids costs. + + The cost is a flat charge per operation -- canonicalizing the call and + digesting it -- so it is invisible next to real signal processing and + plain next to an operation which barely touches the data. Both ends + are timed here, because it is the cheap end which decides whether the + `patch_provenance` knob is worth keeping. + """ + + @pytest.fixture(scope="class") + def tiny_patch(self): + """The smallest patch worth having: all overhead, no work.""" + return dc.Patch( + data=np.ones((2, 2)), + coords={"distance": np.arange(2), "time": np.arange(2)}, + dims=("distance", "time"), + ) + + @pytest.fixture(scope="class") + def big_mask(self, example_patch): + """A mask the fingerprint has to hash, being an array parameter.""" + return np.asarray(example_patch.data) > 0.5 + + @pytest.mark.benchmark + def test_identity_overhead_tiny_patch(self, tiny_patch): + """The flat charge, with nothing else in the way.""" + tiny_patch.transpose() + + @pytest.mark.benchmark + def test_identity_overhead_tiny_patch_disabled(self, tiny_patch): + """The same call with the ids turned off, as the control.""" + with config_context(patch_provenance="disabled"): + tiny_patch.transpose() + + @pytest.mark.benchmark + def test_identity_overhead_array_argument(self, example_patch, big_mask): + """An array parameter is hashed, which is the one real cost.""" + example_patch.where(big_mask) + + @pytest.mark.benchmark + def test_identity_overhead_real_work(self, example_patch): + """Next to actual filtering the charge should not be findable.""" + example_patch.pass_filter(time=(10, 100)) + + @pytest.mark.benchmark + def test_processor_fingerprint(self, example_patch): + """Building an operation and asking it what it is.""" + dc.proc.normalize.op(dim="time").fingerprint diff --git a/docs/tutorial/patch.qmd b/docs/tutorial/patch.qmd index 6c7b03840..eacf77bcf 100644 --- a/docs/tutorial/patch.qmd +++ b/docs/tutorial/patch.qmd @@ -319,6 +319,29 @@ The path is part of the id, so a derived id is not stable across machines. A sto An operation which hands the patch straight back did nothing, and records nothing. Neither id is part of `Patch.equals`: two patches holding the same data are equal however they were made. +### What builds a patch, and what operates on one + +`new`, `update` and `update_attrs` carry both ids through untouched. They are not operations — they are how a patch function assembles its own result — so stamping there would count every operation twice. + +That has a consequence worth knowing: changing data through `new` yourself leaves the ids saying the data is unchanged and the route is the same one. + +```{python} +doubled = patch.new(data=patch.data * 2) + +# It says it is the same data by the same route, because nothing told it otherwise. +assert doubled.attrs.patch_id == patch.attrs.patch_id +assert doubled.attrs.processing_id == patch.attrs.processing_id +``` + +The ids describe what DASCore was asked to do. Work done through [patch functions](processing.qmd) is described; work done by reaching past them is not. Building a patch from arrays rather than from another patch is the honest case, and mints a new `patch_id`: + +```{python} +import numpy as np + +built = dc.Patch(data=np.asarray(patch.data), coords=patch.coords, dims=patch.dims) +assert built.attrs.patch_id != patch.attrs.patch_id +``` + Set `patch_provenance="disabled"` to stop maintaining them: ```{python} diff --git a/scripts/differential_check.py b/scripts/differential_check.py index 30c687f8d..474ac478c 100644 --- a/scripts/differential_check.py +++ b/scripts/differential_check.py @@ -259,8 +259,14 @@ def digest(patch) -> dict: name: _hash(patch.get_array(name)) for name in sorted(patch.coords.coord_map) } # History holds the repr of the arguments, which says nothing about the - # answer, so it is left out. - attrs = patch.attrs.model_dump(exclude={"history", "coords"}) + # answer, so it is left out. The lineage ids go with it, and for a + # sharper reason: this compares a patch against one built by other + # code, and `patch_id` is minted per patch for anything not read from + # a file while `processing_id` names the route rather than the result. + # Left in, every patch would differ and the check would say nothing. + attrs = patch.attrs.model_dump( + exclude={"history", "coords", "patch_id", "processing_id"} + ) return { "dtype": str(data.dtype), "shape": list(data.shape), diff --git a/tests/test_workflow/test_identity.py b/tests/test_workflow/test_identity.py index 23dfafa2e..96059d115 100644 --- a/tests/test_workflow/test_identity.py +++ b/tests/test_workflow/test_identity.py @@ -272,6 +272,36 @@ def test_two_patches_are_two_data(self): first = dc.get_example_patch("random_das") assert first.attrs.patch_id != dc.get_example_patch("random_das").attrs.patch_id + @pytest.mark.parametrize( + "builder", + [ + lambda p: p.new(data=p.data), + lambda p: p.new(data=np.asarray(p.data) * 2), + lambda p: p.update(data=np.asarray(p.data) * 2), + lambda p: p.update_attrs(tag="rebuilt"), + ], + ) + def test_building_a_patch_is_not_operating_on_one(self, patch, builder): + """ + `new` and `update` carry both ids through untouched. + + They are how a patch function assembles its own result, so + stamping here would count every operation twice. The cost is that + changing data through `new` yourself leaves the ids saying it did + not change -- documented in the patch tutorial, and pinned here + because it is a policy rather than an accident. + """ + out = builder(patch) + assert out.attrs.patch_id == patch.attrs.patch_id + assert out.attrs.processing_id == patch.attrs.processing_id + + def test_a_patch_built_from_arrays_is_new_data(self, patch): + """Naming no source, it is not the same data as anything else.""" + built = dc.Patch( + data=np.asarray(patch.data), coords=patch.coords, dims=patch.dims + ) + assert built.attrs.patch_id != patch.attrs.patch_id + def test_an_operation_advances_what_was_done(self, patch): """Which is what the id is for.""" out = patch.normalize("time") From c1a1e5db692169a504a36c01ebdfa65a36c2e902 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Fri, 21 Aug 2026 10:10:37 +0200 Subject: [PATCH 2/2] Answer the review of the measuring The control was measuring the wrong thing: entering `config_context` builds and validates a whole config, and it was doing so inside the timed body. It moves to a fixture. `fingerprint_call` also memoizes, so repeating one call timed a lookup rather than a digest -- 53 us against 77 us on a two-by-two transpose, and a real loop varies its arguments and pays the second. Both are timed now, and the array case gains the control it was missing. `processing_id` goes back into the differential digest. It is a digest of the route rather than a minted value and is the same in both processes, so keeping it catches a call which stopped being stamped or started canonicalizing its arguments differently -- which the data hash cannot see. Only `patch_id` is excluded. 775 calls still compare identical. The policy test started from an unprocessed patch, so preserving the route was satisfied by dropping it; it starts from an operated one. And the prose said the ids are carried untouched, which is not true of someone who names one and sets it. --- benchmarks/test_patch_benchmarks.py | 65 +++++++++++++++++++++++----- docs/tutorial/patch.qmd | 2 +- scripts/differential_check.py | 15 +++---- tests/test_workflow/test_identity.py | 10 +++-- 4 files changed, 70 insertions(+), 22 deletions(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 3d32e5fc9..6bd489514 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -9,6 +9,7 @@ import dascore as dc from dascore.config import config_context from dascore.utils.patch import get_start_stop_step +from dascore.workflow.processor import _FINGERPRINTS @pytest.fixture(scope="module") @@ -435,11 +436,15 @@ class TestIdentityOverhead: """ What maintaining the lineage ids costs. - The cost is a flat charge per operation -- canonicalizing the call and - digesting it -- so it is invisible next to real signal processing and - plain next to an operation which barely touches the data. Both ends - are timed here, because it is the cheap end which decides whether the + The charge is per operation -- canonicalizing the call and digesting + it -- so it is invisible next to real signal processing and plain + next to an operation which barely touches the data. Both ends are + timed, because it is the cheap end which decides whether the `patch_provenance` knob is worth keeping. + + Repeating one call is the cheap case: `fingerprint_call` memoizes, so + the second identical call pays the lookup and not the digest. Real + loops vary their arguments, so the uncached case is timed too. """ @pytest.fixture(scope="class") @@ -456,20 +461,55 @@ def big_mask(self, example_patch): """A mask the fingerprint has to hash, being an array parameter.""" return np.asarray(example_patch.data) > 0.5 + @pytest.fixture() + def ids_disabled(self): + """ + Turn the ids off around a benchmark, not inside it. + + Entering the context builds and validates a whole config, which + is not what the control is supposed to be measuring. + """ + with config_context(patch_provenance="disabled"): + yield + @pytest.mark.benchmark def test_identity_overhead_tiny_patch(self, tiny_patch): - """The flat charge, with nothing else in the way.""" + """The charge on a call which does nothing else, memoized.""" tiny_patch.transpose() @pytest.mark.benchmark - def test_identity_overhead_tiny_patch_disabled(self, tiny_patch): - """The same call with the ids turned off, as the control.""" - with config_context(patch_provenance="disabled"): - tiny_patch.transpose() + def test_identity_overhead_tiny_patch_disabled(self, tiny_patch, ids_disabled): + """The same call with the ids off, as the control.""" + tiny_patch.transpose() + + @pytest.mark.benchmark + def test_identity_overhead_uncached(self, tiny_patch): + """ + The charge with the memo missed, which is what a real loop pays. + + The cache is cleared rather than the arguments varied, so this + times the same call as the memoized benchmark above and the two + differ by the digest alone. + """ + _FINGERPRINTS.clear() + tiny_patch.transpose() + + @pytest.mark.benchmark + def test_identity_overhead_uncached_disabled(self, tiny_patch, ids_disabled): + """The control for the uncached charge, clearing included.""" + _FINGERPRINTS.clear() + tiny_patch.transpose() @pytest.mark.benchmark def test_identity_overhead_array_argument(self, example_patch, big_mask): - """An array parameter is hashed, which is the one real cost.""" + """An array parameter is hashed, which is the one unflat cost.""" + example_patch.where(big_mask) + + @pytest.mark.benchmark + def test_identity_overhead_array_argument_disabled( + self, example_patch, big_mask, ids_disabled + ): + """The control: the same call without hashing the mask.""" example_patch.where(big_mask) @pytest.mark.benchmark @@ -477,6 +517,11 @@ def test_identity_overhead_real_work(self, example_patch): """Next to actual filtering the charge should not be findable.""" example_patch.pass_filter(time=(10, 100)) + @pytest.mark.benchmark + def test_identity_overhead_real_work_disabled(self, example_patch, ids_disabled): + """The control for real work.""" + example_patch.pass_filter(time=(10, 100)) + @pytest.mark.benchmark def test_processor_fingerprint(self, example_patch): """Building an operation and asking it what it is.""" diff --git a/docs/tutorial/patch.qmd b/docs/tutorial/patch.qmd index eacf77bcf..6c742b28a 100644 --- a/docs/tutorial/patch.qmd +++ b/docs/tutorial/patch.qmd @@ -321,7 +321,7 @@ An operation which hands the patch straight back did nothing, and records nothin ### What builds a patch, and what operates on one -`new`, `update` and `update_attrs` carry both ids through untouched. They are not operations — they are how a patch function assembles its own result — so stamping there would count every operation twice. +`new`, `update` and `update_attrs` carry both ids through, unless you name one and set it yourself. They are not operations — they are how a patch function assembles its own result — so stamping there would count every operation twice. That has a consequence worth knowing: changing data through `new` yourself leaves the ids saying the data is unchanged and the route is the same one. diff --git a/scripts/differential_check.py b/scripts/differential_check.py index 474ac478c..61d4ec608 100644 --- a/scripts/differential_check.py +++ b/scripts/differential_check.py @@ -259,14 +259,13 @@ def digest(patch) -> dict: name: _hash(patch.get_array(name)) for name in sorted(patch.coords.coord_map) } # History holds the repr of the arguments, which says nothing about the - # answer, so it is left out. The lineage ids go with it, and for a - # sharper reason: this compares a patch against one built by other - # code, and `patch_id` is minted per patch for anything not read from - # a file while `processing_id` names the route rather than the result. - # Left in, every patch would differ and the check would say nothing. - attrs = patch.attrs.model_dump( - exclude={"history", "coords", "patch_id", "processing_id"} - ) + # answer, so it is left out. `patch_id` goes with it for a harder + # reason: this dumps in two processes, and a patch not read from a + # file mints one, so every patch would differ and the check would say + # nothing. `processing_id` stays -- it is a digest of the route, the + # same in both processes, so it catches a call which stopped being + # stamped or started fingerprinting its arguments differently. + attrs = patch.attrs.model_dump(exclude={"history", "coords", "patch_id"}) return { "dtype": str(data.dtype), "shape": list(data.shape), diff --git a/tests/test_workflow/test_identity.py b/tests/test_workflow/test_identity.py index 96059d115..45119e5dd 100644 --- a/tests/test_workflow/test_identity.py +++ b/tests/test_workflow/test_identity.py @@ -291,9 +291,13 @@ def test_building_a_patch_is_not_operating_on_one(self, patch, builder): not change -- documented in the patch tutorial, and pinned here because it is a policy rather than an accident. """ - out = builder(patch) - assert out.attrs.patch_id == patch.attrs.patch_id - assert out.attrs.processing_id == patch.attrs.processing_id + # Operated on first: an unprocessed patch states no route, so + # preserving it would be satisfied by dropping it. + processed = patch.abs() + assert processed.attrs.processing_id != NOTHING_DONE + out = builder(processed) + assert out.attrs.patch_id == processed.attrs.patch_id + assert out.attrs.processing_id == processed.attrs.processing_id def test_a_patch_built_from_arrays_is_new_data(self, patch): """Naming no source, it is not the same data as anything else."""