🔨 Refactor code-generation - #77
Merged
Merged
Conversation
* The code-generator currently only works for the TA-Lib (C-side) indicators and uses the TA-Lib functions list to identify all indicators with their respective signatures wrapped in X-Macros. The goal is to be independent from BASH and use an algorithmic approach so input arguments that has default values are not silently dropped. Furthermore it is expected that ALL TA-Lib indicators will be generated using X-Macros.
serkor1
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #77 +/- ##
==========================================
- Coverage 96.47% 96.28% -0.20%
==========================================
Files 281 155 -126
Lines 23849 18156 -5693
==========================================
- Hits 23009 17481 -5528
+ Misses 840 675 -165 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* The X-Macros will be using TA_* prefixes for readable preprocessing. So DBL --> TA_DOUBLE for doubles.
* The codegen/ now also parses the TA_<indicator>_Lookback signatures which is the optional inputs for all the functions. * The original C_MACRO has been renamed to BATCH to distinguish between streaming (Upcoming) and batch API for later TA-Lib versions.
* The identifiers follows a simple if-else statement for identifying candlesticks based on the indicator name 'CDL'. With this implementation it is now possible to implement candlestick-specific logic on the C preprocessing side without having to rewrite the logic downstream.
* All lookback related functions have been removed to accomodate the new codegen that uses the exact signature of the TA_<indicato>_lookback funtions.
## 📚 What? This PR is a rewrite of the source code that binds TA-Lib to R. The overall goal is to reduce the amount of code, and simplify the C-wrappers. It has been implemented incrementally, so much of the original functionality as been deleted but is to be reintroduced in a similar fashion. There has been no consideration for backwards compatibility which in this case means that a part of the R logic has to be re-written (This does not affect the signatures at all). All *_lookback functions, for example, now only accepts the relevant arguments in `.Call()` which breaks with the current R side logic. (This should be possible by only rewriting the templates, so the PR is mainly focused on the C-side)
* fmt target now also runs cargo fmt on the crate * gen-code target now includes cargo run * Deleted old script runners
* This commit mainly brings a new backend to the codegeneration on the R side - the old generation were handled by a mix of R and BASH which were harder to maintain than first expected. shQuote() or was it paste()? And how does BASH recieve these arguments? These were questions and debugging the code generation were riddled with. Using Rust unifies the headache in a uniform way, which seems to be easier maintainable.
* The code generation is mainly handled by mining the .xml file that TA-Lib constructs without any modifications to the input arguments. This means two things: (1) breaking changes in R signatures and (2) bug-fixes where MATypes were mixed with multiple indicators (See ta_APO.R) where the MAType's role were ambigious. {talib} is still in pre 1.0.0 and the download counts are so low that there should not be any breakage in dependencies downstream.
* The code generation on the R side is (at this stage) a brute attempt which means that some of the exported functions are either broken or incorrectly named. These will be fixed in the subsequent commits to avoid too many changes in a single commit. This squash commit contains 24 (ish) commits, which makes the development flow non-transparent, which is also why the bugs are kept as-is at this stage.
* The optional documentation is mined fromn the .xml file and manipulated so it emits proper R documentation for each documented parameter.
* This change reflects the new signature the exported functions follow.
* Added entry to the README on the emitted the maType documentation for future reference.
* Instead of mining <Type>-tags, mining <Name>-tags produces more reliable
emission of input columns.
- All columns are now 1:1 mapped instead of a greedy approach which classifies
unmatched columns to 'close'
* The generated R files based on Transforms with more than one series (See TA_CORREL) were only emitting a single 'x' instead of x,y pair.
* The same-ol 0 --> "SMA", 1 --> "EMA" etc. for decorating charts.
* MAVP has its own example to accomodate the additional array of periods needed in the underlying C-signature.
* All output is documented via roxygen2 templates
which fails for MAVP due to missing periods column
+ The addition here avoids messing with the data-raw
files. And documentation is generally generated once
so its basically zero cost.
* This commit is related to 107c25b where arguments were changed.
* Its unclear when the CLAUDE.md files were merged into the {talib}
repository - but R CMD check have not complained about this before now.
* This is a barebone implementation without any modifications mainly added to supress the R CMD check errors. - It will be properly implemented in a later iteration.
* They are to be rewritten in a later iteration - too much AI slop and refactoring to do it meaningfully with the current setup.
* This is a Time Series Forecasting function. I have not studied the source code as per this commit - but as with the remaining regression related functions, I believe that R has a better available suite for this but I am open to include this (or similar functions) if there is demand for it.
* This is a Time Series Forecasting function. I have not studied the source code as per this commit - but as with the remaining regression related functions, I believe that R has a better available suite for this but I am open to include this (or similar functions) if there is demand for it.
* The function helps mapping maTypes back and forth to TA_MA()-functions, and map the types to characters for charting.
* All MA functions returns a maType-class list
if x is missing - this functionality has always
been in {talib} but without the explicit classes
and dispatches.
With the current changes so far, the S3 serves a rather
crucial role in translating the maType-list back and forth
from C and the charts.
It was mainly implemented to fix ta_VOLUME which would
crash due to the C-side extracting the first argument
of the list that was coerced to a integer vector.
* With this new S3 helper charting and new functions
should become easier - this is what we call positive externality
in economics.
* All vignettes render cleanly locally.
* This vignette was written "just" to to show {tidyverse} could
be used for future tidy-people.
The vignette was written purely with AI, without any consideration
for the actual use-case or necessity.
- Off it goes!
* Some indicators requires custom examples to run like, for example, the ta_MAVP - the new boolean flag takes precedence over any auto-generated example.
* This keeps R CMD check happy!
* The helper files are run before the tests starts altogether. So the data can be properly prepared *once* and passed downstream.
* This is an artifact from X-macro PR where it
seemed like a good idea to add void to the C-signature
+ This commit is mostly to satisfy R CMD check rather than
a proper fix on anything. Most of the zzz.R functions onload and unload
are obsolete as its handled on the C-side anyways.
* Re-introduced rolling minimum, maximum and sum.
+ The naming has been changed to accomodate the more
verbose naming.
Merged
* All Math Operators and Statistics Functions are now tagged as Rolling Statistics.
* Some (only one really) functions have an additional passthrough parameter
that does not fit in the OHLC-V schema.
- The specific offender is the MAVP which has a periods argument.
* With this commit any future additions of functions with passthroughs
that are not OHLC-V can be easily integrated into {talib}, without
having to consider a rewrite.
* The MAVP have been regenerated so it accepts the periods parameter. This has introduces a cascade of changes in the downstream files (tests, examples, etc)
* The ROCR were incorrectly excluded during the refactor.
* All functions have a lookback calculation and these should be included by default with their alias and function names.
* This is a work in progress.
Owner
Author
|
Other than NEWS.md being rebased (It was supposed to be squash merged) everything looks fine. |
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
1 participant
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.
📚 What?
The library is currently standing at +300k line additions and 230k line removals. TA-Lib indicators are at 12k lines alone, and the code-generators alone are in a ballpark that is beyond what is maintainable in the long run. Furthermore, the transition from manually coding everything to migrating to BASH for auto-generating the code has introduced a wide variety of bugs (See, example, Issue #39).
To mitigate this I have opened Issue #75, but the {boilerplate.rs} did not really solve the complexity of the code-generation as the
config.toml-file itself varied between 700-1200 lines of code (And this is even with the help AI). This PR introduces the following changes:📋 TODO
Important
We need to implement Candlestick indicators so it candlestick specific logic can be implemented
in the core logic.
This PR closes #75
Footnotes
The bugs, errors and slow compilation time is obviously my own fault. And the rewrite is mainly driven by me wanting to learn new stuff and having a maintainable repository. ↩ ↩2