fix: source unstable-period ids from the C header (#752) - #753
Open
mario4tier wants to merge 1 commit into
Open
Conversation
_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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #752.
Bug
_ta_func_unst_idswas built withenumerate()over the TA-Lib 0.4.0 names.TA-Lib C 0.6.0 inserted
TA_FUNC_UNST_IMIat slot 12, shifting every id fromthere up.
setup.pyrequires ta-lib >= 0.6.1, so every normal install isaffected:
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.
Fix
Ids now come from
lib.TA_FUNC_UNST_*. Cython reads acdef externconst'svalue 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 versiondetection.
'NONE': -1stays a Python literal —TA_FUNC_UNST_NONEno longer exists incurrent headers.
'ADXR','MFI'and'STOCHRSI'remain accepted keys but are now no-opsthat emit a
DeprecationWarning; their enumerators areTA_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 theTA_FUNC_UNST_HD_PHASORtypo, dropped the staleliteral values, and dropped the enumerators that no longer exist.
Test
test_unstable_period_moves_its_own_functioncovers all 20 real ids: setting anunstable 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 awarning/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
masterthe new tests fail exactly wherethe 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:
test_talib_versionassertsb'0.7.1'TA_FUNC_UNST_ALLresolves to 24 on 0.6.x/0.7.x and 65535 on 0.8.x, and thesame 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
masterfails identically.
_abstract.pxi's__get_flagsraisesKeyError: 33554432on
TA_FUNC_FLG_STREAM, a newer function-flag bit missing from its hardcodedTA_FUNC_FLAGSdict — the same class of bug as this one, in another table.talib/_ta_lib.cis left untouched, since it's only committed on Cython versionbumps.