Skip to content

fix: source unstable-period ids from the C header (#752) - #753

Open
mario4tier wants to merge 1 commit into
masterfrom
fix/752-unstable-period-ids
Open

fix: source unstable-period ids from the C header (#752)#753
mario4tier wants to merge 1 commit into
masterfrom
fix/752-unstable-period-ids

Conversation

@mario4tier

Copy link
Copy Markdown
Member

Fixes #752.

Bug

_ta_func_unst_ids was built with enumerate() over the TA-Lib 0.4.0 names.
TA-Lib C 0.6.0 inserted TA_FUNC_UNST_IMI at slot 12, shifting every id from
there up. setup.py requires ta-lib >= 0.6.1, so every normal install is
affected: set_unstable_period('RSI', n) sets PLUS_DM's unstable period,
'MAMA' sets KAMA's, and 'ALL' sets T3's alone.

It went unnoticed because set and get look the id up in the same table, so a
wrong id round-trips perfectly.

name old new what the old id hit
ADX … HT_TRENDMODE 0–11 0–11 correct
KAMA 12 13 IMI
MAMA 13 14 KAMA
MFI 14 no-op MAMA
MINUS_DI 15 16 MFI
MINUS_DM 16 17 MINUS_DI
NATR 17 18 MINUS_DM
PLUS_DI 18 19 NATR
PLUS_DM 19 20 PLUS_DI
RSI 20 21 PLUS_DM
STOCHRSI 21 no-op RSI
T3 22 23 STOCHRSI slot (never read)
ALL 23 24 / 65535 T3 only
ADXR 1 no-op ADXR slot (never read)

Fix

Ids now come from lib.TA_FUNC_UNST_*. Cython reads a cdef extern const's
value from the C header at build time and ignores the literal written in the
.pxd, so one table is correct for every ta-lib release with no version
detection.

  • 'NONE': -1 stays a Python literal — TA_FUNC_UNST_NONE no longer exists in
    current headers.
  • 'ADXR', 'MFI' and 'STOCHRSI' remain accepted keys but are now no-ops
    that emit a DeprecationWarning; their enumerators are TA_FUNC_UNST_UNUSED_*
    in current ta-lib. They are deliberately not aliased to 'ADX'/'RSI'
    that would turn a long-standing no-op into a call with side effects on every
    other function using ADX or RSI internally.
  • _ta_lib.pxd: fixed the TA_FUNC_UNST_HD_PHASOR typo, dropped the stale
    literal values, and dropped the enumerators that no longer exist.

Test

test_unstable_period_moves_its_own_function covers all 20 real ids: setting an
unstable period of 5 must push exactly 5 more leading NaNs — 5 more leading
zeros for the one integer output — into that function's own output, and the
values both runs emit must be identical. Plus test_unstable_period_all, and a
warning/no-op test for the three deprecated names.

A get/set round-trip assertion cannot catch this class of bug; there's a comment
in the test saying so. Against unfixed master the new tests fail exactly where
the numbering diverges: ids 0–11 pass, 12 and up fail.

Verification

Built from source (setup.py build_ext --inplace, Cython 3.2.9, no wheels)
against three ta-lib C versions, same unchanged Python source:

ta-lib C result
0.7.1 70 passed, 0 failed
0.6.4 69 passed, 1 failed — test_talib_version asserts b'0.7.1'
0.8.1 (dev build) 48 passed, 1 failed (same one), plus 2 uncollectable files (below)

TA_FUNC_UNST_ALL resolves to 24 on 0.6.x/0.7.x and 65535 on 0.8.x, and the
same source picks up both — that is what a corrected-constants patch could not do.

The 0.8.1 collection errors are pre-existing and unrelated; unmodified master
fails identically. _abstract.pxi's __get_flags raises KeyError: 33554432
on TA_FUNC_FLG_STREAM, a newer function-flag bit missing from its hardcoded
TA_FUNC_FLAGS dict — the same class of bug as this one, in another table.

talib/_ta_lib.c is left untouched, since it's only committed on Cython version
bumps.

_ta_func_unst_ids was built with enumerate() over the TA-Lib 0.4.0 names.
TA-Lib C 0.6.0 inserted TA_FUNC_UNST_IMI at slot 12, so every id from there
up mis-targets: set_unstable_period('RSI') sets PLUS_DM's unstable period,
'MAMA' sets KAMA's, and 'ALL' sets T3's alone. setup.py requires
ta-lib >= 0.6.1, so this affects every normal install.

Take the ids from lib.TA_FUNC_UNST_* instead. Cython reads the value of a
`cdef extern` const from the header at build time and ignores the literal
written in the .pxd, so one table is correct for every ta-lib release with
no version detection.

'ADXR', 'MFI' and 'STOCHRSI' remain accepted keys but are now no-ops that
emit a DeprecationWarning; TA-Lib C retired those slots. They are not
aliased to the inner ADX/RSI, which would turn a long-standing no-op into a
call with side effects on other functions.

In _ta_lib.pxd, fix the TA_FUNC_UNST_HD_PHASOR typo, drop the stale literal
values, and drop the enumerators that no longer exist.

The new test is behavioural on purpose: set and get look the id up in the
same table, so a wrong id round-trips perfectly.

Verified against ta-lib C 0.6.4, 0.7.1 and 0.8.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set_unstable_period targets the wrong function against TA-Lib >= 0.6.0 (hardcoded enum table)

1 participant