perf: O(1) rolling-sum queues for UltimateHub StreamHub (P010)#1977
perf: O(1) rolling-sum queues for UltimateHub StreamHub (P010)#1977DaveSkender merged 9 commits intov3from
Conversation
Co-authored-by: DaveSkender <[email protected]>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/s-z/Ultimate/Ultimate.StreamHub.cs (1)
94-96: Simplify redundant NaN conversion chain.Line 94 applies
.NaN2Null() ?? double.NaNwhich converts NaN→null→NaN, then line 96 calls.NaN2Null()again. Per guidelines, NaN-to-null conversion should happen only at the final result boundary.♻️ Proposed simplification
// calculate Ultimate Oscillator with weighted average - double ultimate = (100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d).NaN2Null() ?? double.NaN; + double ultimate = 100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d; return (new UltimateResult(item.Timestamp, ultimate.NaN2Null()), i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/Ultimate/Ultimate.StreamHub.cs` around lines 94 - 96, The calculation assigns double ultimate using a redundant NaN-to-null-to-NaN chain and then again converts when constructing the result; remove the intermediate .NaN2Null() ?? double.NaN on the ultimate assignment so the raw numeric double is retained in variable ultimate and only call NaN2Null() once at the final boundary when constructing the UltimateResult (reference symbols: variable ultimate, method NaN2Null(), and constructor UltimateResult(item.Timestamp, ...)). Ensure the final UltimateResult receives ultimate.NaN2Null() and no other NaN2Null/?? double.NaN conversions remain for that value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/streaming-indicators.plan.md`:
- Around line 114-118: The markdown lint error is caused by underscore-prefixed
identifiers being parsed as emphasis; update the plan text to wrap each code
identifier in backticks so they render as inline code (e.g., replace
_shortQueue, _middleQueue, _longQueue, RollbackState, ProviderCache and any
other underscore-prefixed names in that section with `...`). Ensure all
occurrences within the P010 entry (including mentions of running-sum fields and
Series-parity) are consistently backticked so MD037 is resolved.
---
Nitpick comments:
In `@src/s-z/Ultimate/Ultimate.StreamHub.cs`:
- Around line 94-96: The calculation assigns double ultimate using a redundant
NaN-to-null-to-NaN chain and then again converts when constructing the result;
remove the intermediate .NaN2Null() ?? double.NaN on the ultimate assignment so
the raw numeric double is retained in variable ultimate and only call NaN2Null()
once at the final boundary when constructing the UltimateResult (reference
symbols: variable ultimate, method NaN2Null(), and constructor
UltimateResult(item.Timestamp, ...)). Ensure the final UltimateResult receives
ultimate.NaN2Null() and no other NaN2Null/?? double.NaN conversions remain for
that value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/examples/CustomIndicatorsLibrary/AtrWma.csdocs/plans/streaming-indicators.plan.mdsrc/s-z/Ultimate/Ultimate.StreamHub.cstools/application/GlobalSuppressions.cs
💤 Files with no reviewable changes (1)
- tools/application/GlobalSuppressions.cs
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
|
@copilot In the base branch (now merged into this branch) we made a fairly significant set of changes to how rollbacks occur (see PR #1978). Consider if this impacts how you implement the fix for this solution. In any case, resolve the CI check failures and ensure a clean code completion with no lint, build, or test failures. |
Co-authored-by: DaveSkender <[email protected]>
Updated |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/s-z/Ultimate/Ultimate.StreamHub.cs (1)
94-96: Remove redundant NaN↔null conversion in hot path.
ultimateis converted to nullable and immediately converted back todouble, then converted to nullable again. This adds avoidable overhead.♻️ Proposed refactor
- double ultimate = (100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d).NaN2Null() ?? double.NaN; - - return (new UltimateResult(item.Timestamp, ultimate.NaN2Null()), i); + double ultimate = 100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d; + return (new UltimateResult(item.Timestamp, ultimate.NaN2Null()), i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/Ultimate/Ultimate.StreamHub.cs` around lines 94 - 96, Compute ultimate as a plain double (remove the first .NaN2Null() and the "?? double.NaN" coalescing) and only call .NaN2Null() once when constructing the UltimateResult so you don't convert double→nullable→double→nullable in the hot path; e.g., set ultimate = 100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d and return (new UltimateResult(item.Timestamp, ultimate.NaN2Null()), i). This keeps the NaN2Null call only where UltimateResult requires a nullable and avoids the redundant conversions on variable ultimate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/s-z/Ultimate/Ultimate.StreamHub.cs`:
- Around line 94-96: Compute ultimate as a plain double (remove the first
.NaN2Null() and the "?? double.NaN" coalescing) and only call .NaN2Null() once
when constructing the UltimateResult so you don't convert
double→nullable→double→nullable in the hot path; e.g., set ultimate = 100d *
((4d * avg1) + (2d * avg2) + avg3) / 7d and return (new
UltimateResult(item.Timestamp, ultimate.NaN2Null()), i). This keeps the NaN2Null
call only where UltimateResult requires a nullable and avoids the redundant
conversions on variable ultimate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/plans/streaming-indicators.plan.mdsrc/s-z/Ultimate/Ultimate.StreamHub.cstools/application/GlobalSuppressions.cs
💤 Files with no reviewable changes (1)
- tools/application/GlobalSuppressions.cs
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/s-z/Ultimate/Ultimate.StreamHub.cs (1)
94-96: Redundant.NaN2Null()call on line 94.The
.NaN2Null() ?? double.NaNpattern on line 94 is unnecessary. NaN values naturally propagate through arithmetic operations, and the conversion should only happen at the result boundary (line 96). As per coding guidelines: "Convert NaN to null at final result boundary only via .NaN2Null() method."♻️ Proposed fix
- double ultimate = (100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d).NaN2Null() ?? double.NaN; + double ultimate = 100d * ((4d * avg1) + (2d * avg2) + avg3) / 7d; return (new UltimateResult(item.Timestamp, ultimate.NaN2Null()), i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/Ultimate/Ultimate.StreamHub.cs` around lines 94 - 96, Remove the redundant .NaN2Null() call when computing the local variable ultimate so NaN propagation remains until the final boundary; compute ultimate as a plain double (retain the arithmetic using avg1/avg2/avg3), and keep the single .NaN2Null() conversion only when constructing the UltimateResult (new UltimateResult(item.Timestamp, ultimate.NaN2Null())). Ensure the variable name ultimate and the NaN2Null() call on the UltimateResult constructor are the only symbols involved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/s-z/Ultimate/Ultimate.StreamHub.cs`:
- Around line 133-146: The XML documentation for UpdateRollingSum is missing
<param> tags which triggers RCS1141; add <param> elements for each
parameter—queue, capacity, bp, tr, sumBp, and sumTr—describing their purpose and
noting that sumBp and sumTr are passed by ref and updated by the method, placing
these <param> tags between the existing <summary> and the method signature so
the linter recognizes them.
---
Nitpick comments:
In `@src/s-z/Ultimate/Ultimate.StreamHub.cs`:
- Around line 94-96: Remove the redundant .NaN2Null() call when computing the
local variable ultimate so NaN propagation remains until the final boundary;
compute ultimate as a plain double (retain the arithmetic using avg1/avg2/avg3),
and keep the single .NaN2Null() conversion only when constructing the
UltimateResult (new UltimateResult(item.Timestamp, ultimate.NaN2Null())). Ensure
the variable name ultimate and the NaN2Null() call on the UltimateResult
constructor are the only symbols involved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/plans/streaming-indicators.plan.mdsrc/s-z/Ultimate/Ultimate.StreamHub.cstools/application/GlobalSuppressions.cs
💤 Files with no reviewable changes (1)
- tools/application/GlobalSuppressions.cs
UltimateHub.ToIndicatorwith three rolling-sum queues for O(1) per updateRollbackStateoverride with index-based signature (per PR refactor: Centralize RollbackState with index-based signature (P011) #1978)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.