fix: Correct inverted ratio returned by obsolete GetPrs

[DaveSkender]

Closes #2208.

Important

Returned values change for code still calling GetPrs. In 3.0.0 it returned base / eval — the reciprocal of the ratio this library documents. It now returns eval / base. If you called GetPrs on 3.0.x, stored values and any thresholds tuned against them must be revalidated; new values are the reciprocal of old ones. Callers of ToPrs were never affected.

Separately, GetPrs(eval, base) with no lookbackPeriods previously threw ArgumentOutOfRangeException and now computes with a null PrsPercent, as it did before 3.0.0.

The defects

Inverted ratio. The receiver of .ToPrs(..) binds to sourceEval, and the shim passed quotesBase there (Obsolete.V3.Indicators.cs:683-684 on main). Measured on the repo's test data:

GetPrs(eval, base, 20)  = 1.3568166993
ToPrs(eval, base, 20)   = 0.7370192308     <- correct
ToPrs(base, eval, 20)   = 1.3568166993     <- what the shim returned
shim == 1 / correct      : True

Unusable default. lookbackPeriods was mapped to 0, which Prs.Utilities.cs:28 rejects (is <= 0 and not int.MinValue), so the shim's own documented default threw.

Why this is a bug fix, not a behavior change

This restores what the method returned before 3.0.0. It does not choose new behavior.

Before the 3.0.0 rewrite, all three GetPrs overloads called CalcPrs(tpListEval, tpListBase, …) — eval first — and CalcPrs computed Prs = (bValue == 0) ? null : (eValue / bValue), with eValue from tpListEval. (src/m-r/Prs/Prs.Api.cs and src/m-r/Prs/Prs.Series.cs at 896ae089^.) So V2 returned eval ÷ base, and a caller who upgraded to 3.0.0 without changing a line silently began receiving the reciprocal.

Three further corroborations:

• docs/indicators/prs.md defines Prs as Eval / Base, with a worked example
• the shim's own attribute reads "Rename GetPrs(..) to ToPrs(..)", and a rename asserts identical semantics — it cannot carry a deliberate redefinition
• the sibling GetBeta and GetCorrelation shims order their arguments correctly, so this is an isolated slip rather than a pattern

The likely mechanism is visible in the V2 source: it declared tpListBase before tpListEval but passed eval first, and the shim appears to have transcribed declaration order into call order.

The counter-argument, and why it was dismissed

A consumer on 3.0.0 since June may have unknowingly built on the reciprocal, and their numbers move. That was weighed and rejected:

• the values contradicted the library's own published formula, so no consumer could have validated them against the documentation and concluded they were right
• [Obsolete] means the method's entire contract is "behaves as V2 did" — the reciprocal is not a behavior anyone chose
• gating on a major version would mean knowingly shipping wrong numbers for the shim's remaining life, which is the worse trade

This is exactly the case #2215 added to docs/PRINCIPLES.md: correcting a defect, where the shipped behavior never matched its own documentation, is a bug fix and versions as one. It lands in 3.0.1. The disclosure obligation still applies in full — the callout above and the new Corrections to obsolete v2 shims section in docs/migration/v3.md name the affected method, the observable change in output, and the version it lands in.

Worth noting these shims are themselves removed in 3.1. Correcting them still matters, because 3.0.x is the version the migration guide tells users to land on before upgrading further — so it has to be right.

Implementation note

The unspecified lookback now calls the two-argument ToPrs overload, which is the library's own public expression of "compute no PrsPercent", rather than replicating that overload's internal int.MinValue sentinel in the shim. If the sentinel ever changes, the shim follows automatically.

The tuple overload knowingly diverges from V2 on one point: it also accepts an explicit 0, which V2 rejected. The 3.0.0 signature narrowed V2's int? to int = 0, so 0 is the only marker left for "unspecified", and restoring the default has to honor it. Restoring the int? signature instead would break already-compiled callers rather than fix them. Pinned by GetPrsFromTuplesTreatsZeroAsUnspecified.

Tests

These are the first tests to call GetPrs at all. No test anywhere referenced it and every overload carries [ExcludeFromCodeCoverage] — that is why two defects survived a major release.

Ten tests, including absolute anchored values so a co-regression in ToPrs cannot pass unnoticed (every other assertion compares the shim against ToPrs, so both sides would move together), explicit-0 and negative-lookback pins, and a reflection test for the Obsolete(.., true) overload — C# cannot call it and CS0619 is not suppressible, so reflection is the only way it can be verified.

Mutation-verified:

Mutation	Result
re-invert the reachable overload	4 of 10 fail
unspecified lookback back to 0	GetPrsWithUnspecifiedLookbackComputesWithoutPercent fails
break the reflection-only overload	ErrorLevelGetPrsOverloadAppliesTheSameLookbackMapping fails

• dotnet build --no-incremental -c Release — 0 errors, 0 new warnings (15 NU1507 are pre-existing package-source config)
• dotnet test -c Release — 2542 / 76 / 4 passing, 0 failed

Reviewed

Three specialist lanes. The restoration claim was independently verified against the V2 source by two of them. Review changed the implementation (dropping a private sentinel constant in favor of the two-argument overload), added four tests, and struck a rationale of mine that was factually wrong — I had argued the Obsolete(.., true) site was worth patching because CS0619 is suppressible; it is not. The site is still worth patching because reflection reaches it, which the new test demonstrates.

A second, worse defect of the same class is out of scope here and not yet filed: GetPvo's shim declares fastPeriods = 9, slowPeriods = 12 where both V2 and ToPvo declare 12 and 26. bars.GetPvo() returns 1.4387 against a correct 10.4395 — silently, with no exception. Raised for routing rather than folded in, since it needs its own tracked record.

[facioquoqa]

FaciōQuōqa Mage Reviewer is standing by.

• Trigger review

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric	Results
Duplication	0

View in Codacy

🟢 Coverage ∅ diff coverage · -0.01% coverage variation

Metric	Results
Coverage variation	✅ -0.01% coverage variation (-1.00%)
Diff coverage	✅ ∅ diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (84f8285)	19562	18879	96.51%
Head commit (e92820e)	19602 (+40)	18915 (+36)	96.50% (-0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#2210)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}