fix(ops): make IdxMax and IdxMin skip NaN inside the window - #2353
Ludwig J. Marx (LudwigJMarx) wants to merge 3 commits into
Conversation
IdxMax and IdxMin return the position of the extreme value inside the rolling window, but nothing held them to the value Max and Min report for the same window. The new test states that invariant instead of an invented number, so it stays meaningful if the implementation changes. The leaf feature only replaces the data access; the operators under test run their real code. It fails on this commit: with [1.0, 5.0, nan, 2.0, 3.0] and window 3, every window holding a NaN makes IdxMax point at the NaN while Max reports the real maximum. Refs microsoft#2352
ndarray.argmax and ndarray.argmin do not skip NaN, they return its position. Max and Min go through pandas and do skip it, so the two contradicted each other on the same series, and Mad and Rank in the same file already filter NaN explicitly. Alpha158 builds IMAX, IMIN and IMXD on these two operators over $high and $low, which are NaN while an instrument is suspended. On csi300 over 2019, 30 instruments, every one of them holds at least one NaN in $high, and IMAX5 differs in 3.06 percent of its values. No guard against ValueError: All-NaN slice encountered is needed, and that is measured rather than assumed: with min_periods=1 pandas does not call apply at all for a window holding only NaN, on the rolling and on the expanding path. The min_periods=1 sits on the same line as the apply call. Filtering NaN out first, the way Mad and Rank do, was rejected: the returned position would be the one inside the filtered array rather than inside the window, which is exactly what the field means. Refs microsoft#2352
|
@microsoft-github-policy-service agree |
Parafee41 (koriyoshi2041)
left a comment
Parafee41 (koriyoshi2041)
left a comment
There was a problem hiding this comment.
The sibling-operator invariant is a strong regression shape. One branch changed here is not pinned by the new tests, though: both IdxMax and IdxMin switch the N == 0 expanding path to nanarg*, while TestIdxRollingOperator.WINDOW = 3 only exercises rolling.
I replayed the same series with expanding(min_periods=1): the old argmax/argmin path points at NaN from indices 2–4, while the proposed nanargmax/nanargmin path keeps pointing at the sibling Max/Min value. I also confirmed pandas does not invoke the callback for an all-NaN expanding prefix, matching the PR's stated safety assumption.
Could the regression cover both N=3 and N=0 (for example by running the same invariant helper for each)? That would pin the second changed execution path and the expanding-specific all-NaN assumption rather than leaving them supported only by the PR description.
The fix changes four lines: each operator has a rolling call site and an expanding one. WINDOW = 3 only reached the rolling branch, so half of the diff carried no test. Measured rather than assumed. With the expanding branch reverted to argmax and argmin, and the rolling branch left fixed, both tests stayed green: the mutation survived. After the change the same mutation fails 6 subtests, and so does the mirror mutation on the rolling branch. WINDOWS = (3, 0) runs the same invariant helper for both branches, with a subTest per window and index. The helper takes the window size, because the prefix for N == 0 is not the same slice as the window for N > 0. On the expanding path with [1.0, 5.0, nan, 2.0, 3.0], the old code points at the NaN from index 2 to index 4. Max reports 5.0 for each of them. Refs microsoft#2352
|
Confirmed, and the test did not catch it. I reverted the expanding branch to
Mutation results:
The expanding path, measured against the real operators on
Rest of the suite on |
Parafee41 (koriyoshi2041)
left a comment
Parafee41 (koriyoshi2041)
left a comment
There was a problem hiding this comment.
The updated regression now covers both changed branches (N=3 rolling and N=0 expanding) and catches the old arg* implementation in either branch. I reran the exact head locally: 2 tests / 20 subtests pass, and git diff --check is clean.
2 participants
Description
IdxMaxandIdxMinusedndarray.argmax/ndarray.argmin, which returnthe position of a NaN rather than skipping it. Replaced with
np.nanargmax/np.nanargmin, four lines.Motivation and Context
Refs #2352.
MaxandMinskip NaN through pandas,IdxMaxandIdxMindid not, so thetwo disagreed on the same series.
MadandRankin the same file alreadyfilter NaN explicitly.
Alpha158 builds
IMAX{d},IMIN{d}andIMXD{d}on these operators over$highand$low, which are NaN while an instrument is suspended.No guard against
ValueError: All-NaN slice encounteredis needed, and that ismeasured rather than assumed: with
min_periods=1pandas does not callapplyat all for a window that holds only NaN. Checked for both the
rollingand theexpandingpath. Themin_periods=1sits on the same line as theapplycall, so the assumption is local.
Rejected: filtering NaN out first, the way
MadandRankdo. The returnedposition would then be the one inside the filtered array rather than inside the
window, which is exactly the quantity the field means (days since the high).
How Has This Been Tested?
pytest qlib/tests/test_all_pipeline.pyunder upper directory ofqlib.tests/test_all_pipeline.pyis the one that matters here: it runsDATASET_ALPHA158_CLASSover csi300 from 2008 to 2020, so it exercises thefields this change touches, and
test_1_backtestchecks a real threshold.It carries
pytest.mark.slow, so CI skips it. Ran both sides:7bd57c184f5941e0The runtimes differ because
GBDT_MODELsets no seed and usesnum_threads: 20. The test checks a threshold, not equality.New file
tests/ops/test_rolling_operator.py. It asserts no invented numberbut the invariant against the real sibling operators:
IdxMaxhas to point atthe value
Maxreports. The two commits carry the evidence: the test is red on7bd57c18and green on4f5941e0.Before the fix:
After the fix:
Rest of the suite, same command as CI (
cd tests && pytest . -m "not slow"):The two extra are the new tests.
Counter-check that nothing else moves: 200000 random windows without NaN, ties
forced,
argmaxagainstnanargmaxandargminagainstnanargmin, zerodifferences.
black -l 120 --checkclean,flake8clean,pylint10.00/10 onqlib/data/ops.py.Environment: Python 3.12.13, numpy 2.5.3, pandas 2.3.3, MacOS 26.6.2 arm64,
commit be72549.
Types of changes