Force Index Strategy added. #238
Conversation
WalkthroughThe pull request introduces significant updates to the Indicator Go module, particularly enhancing the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #238 +/- ##
==========================================
+ Coverage 92.93% 93.27% +0.34%
==========================================
Files 169 170 +1
Lines 4416 5922 +1506
==========================================
+ Hits 4104 5524 +1420
- Misses 253 337 +84
- Partials 59 61 +2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
strategy/volume/volume.go (1)
26-32: Consider maintaining strategy order for consistencyThe order of strategies in the returned slice has changed with the addition of the new strategy. While this doesn't affect functionality, it might impact consumers who rely on a specific order of strategies. Consider either:
- Adding the new strategy at the end of the slice to maintain the existing order, or
- Documenting that the order of strategies should not be relied upon.
strategy/volume/force_index_strategy_test.go (3)
17-37: LGTM: TestForceIndexStrategy is well-structured and comprehensive.The test function is well-implemented, covering the essential aspects of the ForceIndexStrategy. It reads test data from external files, computes results, and compares them with expected outcomes.
Consider adding a descriptive comment at the beginning of the function to explain its purpose and the strategy being tested. This would enhance the test's documentation and make it easier for other developers to understand the test's intent.
39-55: LGTM: TestForceIndexStrategyReport covers report generation functionality.The test function effectively checks the report generation and file writing capabilities of the ForceIndexStrategy. Good practices such as error handling and file cleanup are implemented.
Consider the following improvements to enhance the test's robustness:
- Verify the contents of the generated HTML file to ensure it contains expected elements or structure.
- Use a temporary directory for file operations instead of the current directory to avoid potential conflicts.
Here's an example of how you could implement these suggestions:
func TestForceIndexStrategyReport(t *testing.T) { snapshots, err := helper.ReadFromCsvFile[asset.Snapshot]("testdata/brk-b.csv", true) if err != nil { t.Fatal(err) } fis := volume.NewForceIndexStrategy() report := fis.Report(snapshots) // Create a temporary directory for the test tempDir, err := os.MkdirTemp("", "force_index_test") if err != nil { t.Fatal(err) } defer os.RemoveAll(tempDir) // Clean up fileName := filepath.Join(tempDir, "force_index_strategy.html") err = report.WriteToFile(fileName) if err != nil { t.Fatal(err) } // Read the generated file content, err := os.ReadFile(fileName) if err != nil { t.Fatal(err) } // Verify the content (adjust these checks based on your expected output) if !strings.Contains(string(content), "<html>") { t.Error("Generated report doesn't contain HTML structure") } if !strings.Contains(string(content), "Force Index Strategy") { t.Error("Generated report doesn't mention Force Index Strategy") } }This improved version uses a temporary directory, reads the generated file, and performs basic checks on its content.
1-55: Overall, the test file is well-structured and provides good coverage.The
force_index_strategy_test.gofile effectively tests both the computation and report generation aspects of the ForceIndexStrategy. It follows good testing practices such as using external test data, proper error handling, and cleanup of resources.To further enhance the test suite:
- Consider adding edge cases or boundary conditions to ensure the strategy behaves correctly in all scenarios.
- If not already present in other files, consider adding integration tests that use the ForceIndexStrategy in combination with other components of the system.
- Ensure that these tests are included in any continuous integration pipelines to maintain the reliability of the ForceIndexStrategy as the project evolves.
README.md (1)
142-142: Replace hard tab with spaces for consistency.The static analysis tool detected a hard tab on this line. To maintain consistency with the rest of the document, please replace the hard tab with spaces.
Apply this change:
-- [Force Index Strategy](strategy/volume/README.md#type-forceindexstrategy) +- [Force Index Strategy](strategy/volume/README.md#type-forceindexstrategy)🧰 Tools
🪛 Markdownlint
142-142: Column: 2
Hard tabs(MD010, no-hard-tabs)
strategy/volume/README.md (1)
144-200: LGTM! Consider adding a brief explanation of the Force Index indicator.The new ForceIndexStrategy section is well-structured and consistent with the documentation of other strategies. The description of the strategy and its methods is accurate and complete.
To further improve the documentation, consider adding a brief explanation of what the Force Index indicator represents and how it's calculated. This would provide more context for users unfamiliar with this particular indicator.
strategy/volume/force_index_strategy.go (2)
39-39: Clarify the period calculation in theNamemethodIn the
Namemethod, adding 1 tof.ForceIndex.IdlePeriod()may cause confusion about the actual period used. Review this calculation to ensure it accurately reflects the intended strategy period or adjust the formatting for clarity.
18-21: Add documentation for theForceIndexfieldCurrently, the
ForceIndexfield in theForceIndexStrategystruct lacks a descriptive comment. Adding documentation will improve code readability and maintainability.Apply this diff to add the field comment:
type ForceIndexStrategy struct { + // ForceIndex computes the Force Index indicator used in the strategy. ForceIndex *volume.Fi[float64] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
strategy/volume/testdata/force_index_strategy.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
- README.md (1 hunks)
- strategy/volume/README.md (2 hunks)
- strategy/volume/force_index_strategy.go (1 hunks)
- strategy/volume/force_index_strategy_test.go (1 hunks)
- strategy/volume/volume.go (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
142-142: Column: 2
Hard tabs(MD010, no-hard-tabs)
🔇 Additional comments (7)
strategy/volume/volume.go (1)
29-29: LGTM: Force Index Strategy successfully addedThe addition of
NewForceIndexStrategy()to theAllStrategies()function is correct and aligns with the PR objectives. This change successfully introduces the Force Index Strategy to the available volume strategies.strategy/volume/force_index_strategy_test.go (1)
7-15: LGTM: Import statements are appropriate and well-organized.The import statements are correctly grouped and include all necessary packages for the test file's functionality.
README.md (2)
142-142: LGTM: Force Index Strategy added successfully.The addition of the Force Index Strategy to the Volume Strategies section is consistent with the PR objectives and enhances the documentation. The link format and placement are correct.
🧰 Tools
🪛 Markdownlint
142-142: Column: 2
Hard tabs(MD010, no-hard-tabs)
Line range hint
1-324: LGTM: Comprehensive update to README.mdThe README.md file has been significantly improved to reflect the enhancements in version 2 of the Indicator Go module. The structure is well-organized, and the content is comprehensive, covering all major aspects of the module including:
- Lists of indicators and strategies
- New sections on repositories and backtesting
- Installation and usage instructions
- Licensing information
These updates align well with the PR objectives and the AI-generated summary.
🧰 Tools
🪛 Markdownlint
140-140: Column: 2
Hard tabs(MD010, no-hard-tabs)
141-141: Column: 2
Hard tabs(MD010, no-hard-tabs)
142-142: Column: 2
Hard tabs(MD010, no-hard-tabs)
143-143: Column: 2
Hard tabs(MD010, no-hard-tabs)
144-144: Column: 2
Hard tabs(MD010, no-hard-tabs)
145-145: Column: 2
Hard tabs(MD010, no-hard-tabs)
strategy/volume/README.md (2)
35-40: LGTM! ForceIndexStrategy is well-integrated into the index.The ForceIndexStrategy has been correctly added to the index section, maintaining consistency with other strategy entries. The alphabetical order of strategies in the index has been preserved, which aids in easy navigation of the documentation.
Line range hint
1-200: Overall, the changes effectively introduce the ForceIndexStrategy.The addition of the ForceIndexStrategy to the volume package documentation is well-executed. The new strategy is properly documented and integrated into the existing structure of the README. These changes align with the PR objectives of introducing the Force Index Strategy.
The documentation remains consistent and well-structured, maintaining the high quality of the existing content. This update enhances the comprehensiveness of the volume strategies documentation, providing users with information about another useful trading strategy.
strategy/volume/force_index_strategy.go (1)
64-64: Verify correct alignment after shifting actions byIdlePeriodShifting the actions by
f.ForceIndex.IdlePeriod()adjusts for the initial period without sufficient data. Verify that this shift correctly aligns the actions with the corresponding snapshots to ensure accurate strategy recommendations.Run the following script to confirm alignment:
| // Compute processes the provided asset snapshots and generates a stream of actionable recommendations. | ||
| func (f *ForceIndexStrategy) Compute(snapshots <-chan *asset.Snapshot) <-chan strategy.Action { | ||
| snapshotsSplice := helper.Duplicate(snapshots, 2) | ||
|
|
||
| closings := asset.SnapshotsAsClosings(snapshotsSplice[0]) | ||
| volumes := asset.SnapshotsAsVolumes(snapshotsSplice[1]) | ||
|
|
||
| fis := f.ForceIndex.Compute(closings, volumes) | ||
|
|
||
| actions := helper.Map(fis, func(fi float64) strategy.Action { | ||
| if fi > 0 { | ||
| return strategy.Buy | ||
| } | ||
|
|
||
| if fi < 0 { | ||
| return strategy.Sell | ||
| } | ||
|
|
||
| return strategy.Hold | ||
| }) | ||
|
|
||
| // Force Index starts only after a full period. | ||
| actions = helper.Shift(actions, f.ForceIndex.IdlePeriod(), strategy.Hold) | ||
|
|
||
| return actions | ||
| } |
There was a problem hiding this comment.
Add error handling for potential channel issues in Compute
In the Compute method, there is no error handling for potential issues with the snapshots channel, such as premature closure or receiving nil values. Consider implementing error checks to handle such cases gracefully and prevent runtime panics.
| func (f *ForceIndexStrategy) Report(c <-chan *asset.Snapshot) *helper.Report { | ||
| // | ||
| // snapshots[0] -> dates | ||
| // snapshots[1] -> closings[0] -> closings | ||
| // closings[1] -> force index | ||
| // snapshots[2] -> volumes | ||
| // snapshots[3] -> actions -> annotations | ||
| // -> outcomes | ||
| // | ||
| snapshots := helper.Duplicate(c, 4) | ||
|
|
||
| dates := helper.Skip(asset.SnapshotsAsDates(snapshots[0]), f.ForceIndex.IdlePeriod()) | ||
|
|
||
| closingsSplice := helper.Duplicate( | ||
| asset.SnapshotsAsClosings(snapshots[1]), | ||
| 2, | ||
| ) | ||
| volumes := asset.SnapshotsAsVolumes(snapshots[2]) | ||
|
|
||
| fis := f.ForceIndex.Compute(closingsSplice[0], volumes) | ||
|
|
||
| closingsSplice[1] = helper.Skip(closingsSplice[1], f.ForceIndex.IdlePeriod()) | ||
|
|
||
| actions, outcomes := strategy.ComputeWithOutcome(f, snapshots[3]) | ||
| actions = helper.Skip(actions, f.ForceIndex.IdlePeriod()) | ||
| outcomes = helper.Skip(outcomes, f.ForceIndex.IdlePeriod()) | ||
|
|
||
| annotations := strategy.ActionsToAnnotations(actions) | ||
| outcomes = helper.MultiplyBy(outcomes, 100) | ||
|
|
||
| report := helper.NewReport(f.Name(), dates) | ||
| report.AddChart() | ||
| report.AddChart() | ||
|
|
||
| report.AddColumn(helper.NewNumericReportColumn("Close", closingsSplice[1])) | ||
| report.AddColumn(helper.NewNumericReportColumn("Force Index", fis), 1) | ||
| report.AddColumn(helper.NewAnnotationReportColumn(annotations), 0, 1) | ||
|
|
||
| report.AddColumn(helper.NewNumericReportColumn("Outcome", outcomes), 2) | ||
|
|
||
| return report |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the Report method for improved readability
The Report method contains multiple duplications and skips, which might affect readability and maintainability. Consider refactoring to simplify the data flow, possibly by extracting common patterns into helper functions or restructuring the method for clarity.
| // NewForceIndexStrategy function initializes a new Force Index strategy instance with the default parameters. | ||
| func NewForceIndexStrategy() *ForceIndexStrategy { | ||
| return NewForceIndexStrategyWith( | ||
| volume.DefaultFiPeriod, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add input validation for period parameter in NewForceIndexStrategyWith
The constructor NewForceIndexStrategyWith allows custom period specification but does not validate the input parameter. Consider adding validation to ensure that the period is a positive integer greater than zero to prevent potential runtime errors.
Apply this diff to add input validation:
func NewForceIndexStrategyWith(period int) *ForceIndexStrategy {
+ if period <= 0 {
+ panic("period must be a positive integer greater than zero")
+ }
return &ForceIndexStrategy{
ForceIndex: volume.NewFiWithPeriod[float64](period),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewForceIndexStrategy function initializes a new Force Index strategy instance with the default parameters. | |
| func NewForceIndexStrategy() *ForceIndexStrategy { | |
| return NewForceIndexStrategyWith( | |
| volume.DefaultFiPeriod, | |
| ) | |
| } | |
| // NewForceIndexStrategy function initializes a new Force Index strategy instance with the default parameters. | |
| func NewForceIndexStrategy() *ForceIndexStrategy { | |
| return NewForceIndexStrategyWith( | |
| volume.DefaultFiPeriod, | |
| ) | |
| } | |
| func NewForceIndexStrategyWith(period int) *ForceIndexStrategy { | |
| if period <= 0 { | |
| panic("period must be a positive integer greater than zero") | |
| } | |
| return &ForceIndexStrategy{ | |
| ForceIndex: volume.NewFiWithPeriod[float64](period), | |
| } | |
| } |
Describe Request
Force Index Strategy added.
Change Type
New strategy.
Summary by CodeRabbit
New Features
ForceIndexStrategy, expanding the library's volume strategies.Bug Fixes
Documentation