Flip ResultFloat/ResultVec/ResultBool default from 0 to NaN#952
Merged
Conversation
Contributor
Reviewer's GuideSwitches the default value for numeric result variables from 0 to NaN so unrecorded samples are treated as missing instead of real zeros, updates tests/docs/cache-version and golden hashes accordingly, and keeps ResultBool semantics unchanged. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Now that
ResultFloat/ResultVecdefault toNaN, consider centralising and explicitly handlingNaNin any JSON-serialisation layer (e.g. normalising to a consistent representation rather than relying on default encoder behaviour) so callers don’t run into subtle consumer issues with non-JSON-safe values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `ResultFloat`/`ResultVec` default to `NaN`, consider centralising and explicitly handling `NaN` in any JSON-serialisation layer (e.g. normalising to a consistent representation rather than relying on default encoder behaviour) so callers don’t run into subtle consumer issues with non-JSON-safe values.
## Individual Comments
### Comment 1
<location path="test/test_result_nan_default.py" line_range="60-69" />
<code_context>
class TestNanDefaultConstruction(unittest.TestCase):
- def test_default_is_zero_for_backward_compat(self):
- self.assertEqual(ResultFloat().default, 0)
- self.assertEqual(ResultVec(size=2).default, 0)
+ def test_default_is_nan(self):
+ self.assertTrue(math.isnan(ResultFloat().default))
+ self.assertTrue(math.isnan(ResultVec(size=2).default))
def test_default_can_be_nan(self):
self.assertTrue(math.isnan(ResultFloat(default=float("nan")).default))
self.assertTrue(math.isnan(ResultVec(size=2, default=float("nan")).default))
+ def test_explicit_zero_default_opt_out(self):
+ self.assertEqual(ResultFloat(default=0).default, 0)
+ self.assertEqual(ResultVec(size=2, default=0).default, 0)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for ResultBool to lock in its intentionally different default (0/False) and ensure it does not regress to NaN.
Please add assertions that capture this behaviour, for example:
- `self.assertEqual(ResultBool().default, 0)` and `self.assertFalse(ResultBool().default)`
- Optionally, `self.assertFalse(math.isnan(ResultBool().default))` to guard against an accidental switch to NaN.
That will make the intended difference between numeric and boolean defaults explicit in the tests and help prevent future regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Performance Report for
|
| Metric | Value |
|---|---|
| Total tests | 1451 |
| Total time | 117.38s |
| Mean | 0.0809s |
| Median | 0.0010s |
Top 10 slowest tests
| Test | Time (s) |
|---|---|
test.test_bench_examples.TestBenchExamples::test_example_meta |
15.620 |
test.test_over_time_save_perf::test_save_faster_without_aggregated_tab |
5.040 |
test.test_hash_persistent.TestCrossProcessDeterminism::test_hash_stable_across_two_processes[ResultBool] |
3.961 |
test.test_split_render_examples::test_split_render_subprocess_media |
3.841 |
test.test_generated_examples::test_generated_example[cartesian_animation/example_cartesian_animation.py] |
3.108 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_drift.py] |
2.782 |
test.test_generated_examples::test_generated_example[result_types/result_image/example_result_image_to_video.py] |
2.694 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_step.py] |
2.637 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_noise.py] |
2.261 |
test.test_over_time_repeats.TestMaxSliderPoints::test_default_subsampling_caps_at_max |
2.214 |
Updated by Performance Tracking workflow
Owner
Author
|
Addressed Sourcery review (commit 91eae7d):
|
blooop
force-pushed
the
flip-result-default-to-nan
branch
from
91eae7d to
85af908
Compare
blooop
changed the title
Performance Report for
|
| Metric | Value |
|---|---|
| Total tests | 1479 |
| Total time | 126.97s |
| Mean | 0.0858s |
| Median | 0.0020s |
Top 10 slowest tests
| Test | Time (s) |
|---|---|
test.test_bench_examples.TestBenchExamples::test_example_meta |
18.286 |
test.test_over_time_save_perf::test_save_faster_without_aggregated_tab |
5.551 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_drift.py] |
4.644 |
test.test_generated_examples::test_generated_example[cartesian_animation/example_cartesian_animation.py] |
3.257 |
test.test_split_render_examples::test_split_render_subprocess_media |
3.164 |
test.test_hash_persistent.TestCrossProcessDeterminism::test_hash_stable_across_two_processes[ResultBool] |
2.879 |
test.test_generated_examples::test_generated_example[result_types/result_image/example_result_image_to_video.py] |
2.839 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_step.py] |
2.830 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_noise.py] |
2.797 |
test.test_over_time_repeats.TestMaxSliderPoints::test_default_subsampling_caps_at_max |
2.468 |
Updated by Performance Tracking workflow
An *unrecorded* sample — a run that aborts before measuring, or a result var the worker never sets — was indistinguishable from a real 0/False measurement, dragging nan-aware regression/aggregation means toward zero. The storage layer already initialises result arrays with NaN; the 0 param default was the value that overwrote that "unwritten" sentinel. Flip the default to NaN for ResultFloat, ResultVec, and ResultBool so unrecorded samples stay missing and are dropped by the existing nan-aware reductions (skipna xarray reductions, _clean_1d in regression, np.nanmean/ nansum in the agg registry, df.dropna() in the Optuna path). Callers wanting the old zero/False fill opt out with default=0. ResultBool is included: missing is not a recorded failure, so an unrecorded repeat is dropped from the success proportion rather than counted as False. The binomial SE already divides p*(1-p) by the per-cell count of valid (non-NaN) repeats, so missing repeats don't understate it. A worker that wants a crash/abort to count as failure must record False on its failure path. CACHE_VERSION is deliberately NOT bumped: default is not part of BenchCfg.hash_persistent(), so existing benchmark and over_time history caches are preserved. The new NaN default applies only to cache misses; already-cached cells keep their stored value, so a missing-sample benchmark may transiently mix 0 (old) and NaN (new) until those cells recompute. Version 1.104.2 -> 1.105.0.
blooop
force-pushed
the
flip-result-default-to-nan
branch
from
85af908 to
992f181
Compare
Performance Report for
|
| Metric | Value |
|---|---|
| Total tests | 1479 |
| Total time | 120.52s |
| Mean | 0.0815s |
| Median | 0.0020s |
Top 10 slowest tests
| Test | Time (s) |
|---|---|
test.test_bench_examples.TestBenchExamples::test_example_meta |
16.647 |
test.test_over_time_save_perf::test_save_faster_without_aggregated_tab |
5.281 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_drift.py] |
4.500 |
test.test_generated_examples::test_generated_example[cartesian_animation/example_cartesian_animation.py] |
3.174 |
test.test_split_render_examples::test_split_render_subprocess_media |
2.864 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_step.py] |
2.784 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_noise.py] |
2.761 |
test.test_hash_persistent.TestCrossProcessDeterminism::test_hash_stable_across_two_processes[ResultBool] |
2.755 |
test.test_generated_examples::test_generated_example[result_types/result_image/example_result_image_to_video.py] |
2.712 |
test.test_over_time_repeats.TestMaxSliderPoints::test_default_subsampling_caps_at_max |
2.375 |
Updated by Performance Tracking workflow
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Flip the default value of
ResultFloat,ResultVec, andResultBoolfrom0toNaN.Why
A
0/Falsedefault makes an unrecorded sample — a run that aborts before measuring, or a result var the worker never sets — indistinguishable from a real0/Falsemeasurement, dragging nan-aware regression/aggregation means toward zero. The storage layer (result_collector.setup_dataset) already initialises result arrays withnp.nan; the0param default was the value that overwrote that "unwritten" sentinel.After this change, unrecorded samples stay
NaNand are dropped by the existing nan-aware reductions (skipna=Truexarray reductions,_clean_1dinregression.py,np.nanmean/nansumin the agg registry,df.dropna()in the Optuna path).Scope / decisions
ResultBoolis now included. An earlier draft kept it at0/False, justified by the binomial-std calc treating bool means as proportions over a fixed repeat count. That rationale is now obsolete: the binomial SE was fixed onmainto dividep*(1-p)by the per-cell count of valid (non-NaN) repeats, so missing repeats no longer understate it. The semantics are missing ≠ failure: an unrecorded repeat is dropped from the success proportion rather than counted asFalse. A worker that wants a crash/abort to count as a failure must explicitly recordFalseon its failure path.default=0.CACHE_VERSIONis deliberately NOT bumped. The result-vardefaultis not part ofBenchCfg.hash_persistent(), so existing benchmark andover_timehistory caches are preserved (no forced global wipe). The trade-off: the newNaNdefault only applies to cache misses; already-cached cells keep whatever sentinel they were stored with, so a benchmark with missing samples may transiently hold a mix of0(old) andNaN(new) until those cells are recomputed. For rollingover_timehistory this self-heals as old samples age out of the window.Verification
pixi run cigreen (1474 passed, 5 skipped); format/lint clean (pylint 10.00/10). Noallow_nan=FalseJSON sinks exist; the report-export contract coerces non-finite floats tonullvia_finite_or_none.🤖 Generated with Claude Code