BUG: Stop StepM when all models have been selected

[pedrosfaria2]

Fix StepM termination when all models are selected across multiple rounds

Fixes #862

Summary

This fixes the failure reported in #862 by checking the cumulative number of selected models instead of the number selected in the latest round:

-        while better_models and (len(better_models) < self.k):
+        while better_models and (len(all_better_models) < self.k):

Once the last remaining models have been selected, StepM now leaves the loop, restores the full selector, and assigns _superior_models. Previously, it attempted another SPA calculation with an empty subset and raised a ValueError. The reproduction and traceback are in the issue.

The PR also adds a regression test that requires all models to be selected over multiple rounds. No statistical calculation inside an SPA round is changed.

Stopping condition

Connection to the published stepwise procedure

Romano and Wolf's Algorithm 3.1, p. 1247, defines $R_j$ as the total number of hypotheses rejected so far. The next selection step considers only positions $R_j + 1$ through $S$, where $S$ is the original number of hypotheses. Algorithm 4.1, p. 1252, uses the same cumulative bookkeeping for the studentized procedure. Romano and Wolf (2005), original article

In this implementation, self.k corresponds to $S$, while len(all_better_models) corresponds to $R_j$. Once that cumulative count reaches self.k, no hypotheses remain for another step. The corrected guard implements this terminal state.

The same cumulative structure also appears in the Step-SPA extension: rejected hypotheses are removed before the next critical value is computed over the remaining family. Section 2.2 describes this iteration, and Theorem 2.2 states its asymptotic properties under Assumption 2.1 and a significance level below one half. Hsu, Hsu, and Kuan, author manuscript, Section 2.2 and Theorem 2.2

When the loop should stop

Let $K = {0, \ldots, k - 1}$ be the original model indices, $D_j$ the discoveries in round $j$, and $C_j$ their cumulative union. The active models after that round are

$$ \begin{aligned} C_j &= \bigcup_{i=1}^{j} D_i, \\ A_{j+1} &= K \setminus C_j. \end{aligned} $$

SPA.better_models() restricts discoveries to the active selector. Since StepM excludes all previous discoveries before the next round, discoveries from different rounds are disjoint. Therefore all_better_models contains unique original model indices, and

$$ \begin{aligned} 0 &\leq \lvert D_j \rvert \leq \lvert C_j \rvert \leq k, \\ A_{j+1} \neq \varnothing &\iff \lvert C_j \rvert < k. \end{aligned} $$

Here $\lvert C_j \rvert = R_j$, the cumulative number of rejections in Romano and Wolf's notation.

The loop should stop when the latest round finds no additional superior models or when no models remain. The corrected condition checks both:

while better_models and (len(all_better_models) < self.k):

Preservation of existing results

Write $s$ for len(better_models) and $a$ for len(all_better_models). Since $0 \leq s \leq a \leq k$, the two conditions compare as follows:

State	Existing condition	Corrected condition
$s = 0$	Stop	Stop
$s > 0$ and $a < k$	Continue	Continue
$s = a = k$	Stop	Stop
$0 < s < k$ and $a = k$	Continue with an empty family	Stop

Only the last case changes. It occurs when all models have been selected across multiple rounds: the cumulative set is complete, but the latest round contained fewer than k models. The original condition therefore enters one additional iteration even though the active family is empty.

For every valid nonterminal state, both conditions take the same branch, so the sequence of SPA calculations is unchanged.

Statistical interpretation

Every returned model passes the existing SPA.better_models(self.size) criterion in a nonempty active family. The corrected guard does not alter this criterion or any of the preceding selection rounds.

In the terminal case addressed by this PR, every model has already been selected by one of the preceding nonempty SPA rounds. There is therefore no remaining family on which to compute another critical value. Returning the accumulated decisions completes the procedure without changing any rejection criterion.

Let $I_0$ denote the true null hypotheses and $R$ the set rejected by the completed stepwise procedure. The familywise error event is

$$ R \cap I_0 \neq \varnothing. $$

The fix does not add any rejection to $R$ and does not skip a test on an active hypothesis. It only prevents an additional computation after the active family has become empty.

Romano and Wolf's Theorems 3.1 and 4.1 establish asymptotic familywise error control under their distributional and bootstrap assumptions. Romano and Wolf (2005), Theorems 3.1 and 4.1

For Step-SPA, the stated assumptions include stationarity, suitable weak dependence and moment conditions, and nondegenerate variation. Appropriate resampling remains part of the statistical setup. Hsu, Hsu, and Kuan, Assumption 2.1

Regression test

test_all_superior_multiple_rounds centers, shifts, and scales the existing model-loss data to produce positive mean improvements of different magnitudes. With studentize=False and a fixed bootstrap seed, it verifies that the first round selects a nonempty proper subset and that StepM ultimately returns all models.

This specifically requires multiple selection rounds, which the existing test_all_superior case does not exercise.

The test raises the reported empty-array ValueError with the original stopping condition and passes with the fix.

References

1. Romano, J. P., and Wolf, M. (2005). Stepwise Multiple Testing as Formalized Data Snooping. Econometrica, 73(4), 1237–1282. DOI. Full text. Relevant locations: Algorithm 3.1, p. 1247; Theorem 3.1, p. 1248; Algorithm 4.1, p. 1252; Theorem 4.1, p. 1253.

2. Hsu, P.-H., Hsu, Y.-C., and Kuan, C.-M. (2010). Testing the predictive ability of technical analysis using a new stepwise test without data snooping bias. Journal of Empirical Finance, 17(3), 471–484. DOI. Author manuscript dated July 20, 2009. Relevant locations in that manuscript: Assumption 2.1, Section 2.2, and Theorem 2.2.

3. Published implementation: StepM.compute(), SPA.compute(), and SPA.better_models().

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (c1e37e2) to head (d8e0da3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #863   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          78       78           
  Lines       15815    15824    +9     
  Branches     1294     1294           
=======================================
+ Hits        15743    15752    +9     
  Misses         38       38           
  Partials       34       34           

Flag	Coverage Δ
adder	99.50% <100.00%> (+<0.01%)	⬆️
subtractor	99.50% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bashtage]

Thanks for the bug report and the quick fix.