fix one-in forecaster map wiring and remove dead forecast map code - #927
Open
xmariachi wants to merge 1 commit into
Open
fix one-in forecaster map wiring and remove dead forecast map code#927xmariachi wants to merge 1 commit into
xmariachi wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 2 files
Linked issue analysis
Linked issue: ENGN-5777: inference_synthesis: clean one-in forecaster map wiring and remove dead local forecast map
| Status | Acceptance criteria | Notes |
|---|---|---|
| One-in forecaster outputs remain unchanged for existing epoch fixture tests. | Existing epoch test remains but no explicit regression check shown | |
| ✅ | No panic when a forecaster key in ForecasterToForecastImpliedInference has a nil value. | Added test that sets a nil entry and asserts NotPanics |
| ✅ | singleForecasterToForecast no longer exists in calcOneInValue. | Removed creation and use of singleForecasterToForecast |
| Internal one-in call path compiles and tests pass. | Call wiring updated but test run / CI pass not shown in diffs | |
| ✅ | Pass singleForecastImpliedInference into calcWeightedInference for one-in computation. | One-in path now uses singleForecastImpliedInference in args |
| ✅ | Remove unused local singleForecasterToForecast. | Local singleForecasterToForecast map creation removed |
| ✅ | Remove unused internal arg field ForecasterToForecast from calcOneInValueArgs and call wiring. | Field removed from struct and removed where args are assembled |
| ✅ | Add nil-safe construction for singleForecastImpliedInference to avoid present-but-nil map entry hazards. | Checks ok && inferred != nil before adding to map |
| ✅ | Add regression test for one-in path nil-entry handling. | New test verifies nil-entry handling and ensures no panic |
| Preserve behavior for existing valid inputs while hardening edge handling. | Code intends to preserve behavior but diffs lack explicit regression output comparison |
Architecture diagram
sequenceDiagram
participant Manager as Inference Synthesis Manager
participant Loop as GetOneInForecasterInferences()
participant Calc as calcOneInValue()
participant Weights as calcWeightedInference()
Note over Manager,Weights: One-In Forecaster Evaluation Flow
Manager->>Loop: Get inferences for all forecasters
loop for each OneInForecaster in Forecasters
Loop->>Calc: Calculate value for specific forecaster
Calc->>Calc: NEW: Initialize singleForecastImpliedInference map
alt NEW: Forecaster has valid implied inference
Calc->>Calc: Add forecaster's inference to isolated map
else Forecaster entry is nil or missing
Note right of Calc: Regression fix: Prevent panic by skipping map insertion
end
Calc->>Calc: Isolate singleForecasterRegret and worker list
Calc->>Weights: CHANGED: Call with scoped singleForecastImpliedInference
Note over Calc,Weights: Previously passed full global map; now restricted to current forecaster
Weights-->>Calc: Weighted result for this forecaster
Calc-->>Loop: One-in value
end
Loop-->>Manager: Return collection of one-in inferences
Contributor
|
I believe I already fixed this in the classification PR. let's leave this for later, we could still use the unit test |
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
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.
Purpose of Changes and their Description
Summary
singleForecastImpliedInferenceincalcOneInValueso one-in computation is scoped to the selected forecaster.singleForecasterToForecastand unused internal arg field).Validation
go test ./x/emissions/keeper/inference_synthesis -count=1
Link(s) to Ticket(s) or Issue(s) resolved by this PR
Are these changes tested and documented?
Tests pass, added test
no need, no func change
Unreleasedsection ofCHANGELOG.md?no need, no func change
Summary by cubic
Aligns one-in forecaster computation to the selected forecaster’s forecast-implied inference and remove dead forecast map wiring. Addresses Linear ENGN-5777 by fixing map wiring and hardening nil-entry handling.
Bug Fixes
Refactors
Written for commit e34054d. Summary will update on new commits.