Add Fisher Transform indicator #49
Conversation
--- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/mihakralj/QuanTAlib?shareId=XXXX-XXXX-XXXX-XXXX).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
Tests/test_updates_oscillators.cs(1 hunks)lib/oscillators/Fisher.cs(1 hunks)lib/oscillators/_list.md(2 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
lib/oscillators/Fisher.cs
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
🪛 GitHub Check: CodeFactor
lib/oscillators/Fisher.cs
[notice] 66-66: lib/oscillators/Fisher.cs#L66
Arithmetic expressions should declare precedence. (SA1407)
🪛 LanguageTool
lib/oscillators/_list.md
[grammar] ~2-~2: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: # Oscillators indicators Done: 22, Todo: 7 ✔️ AC - Acceleration Oscillator ✔️ ...
(TO_DO_HYPHEN)
🔇 Additional comments (2)
lib/oscillators/_list.md (1)
18-18: LGTM! Fisher Transform status correctly updated.
The checkmark for Fisher Transform has been added, maintaining alphabetical order in the list. This matches the PR objective of implementing the Fisher Transform indicator.
Let's verify the implementation matches this status update:
✅ Verification successful
Fisher Transform implementation is complete and properly tested
The implementation matches the status update in _list.md. The code includes:
- A complete Fisher Transform implementation in
lib/oscillators/Fisher.cswith proper documentation and formula - A corresponding test case in
Tests/test_updates_oscillators.csthat verifies the indicator's behavior - The implementation follows the same pattern as other oscillators in the codebase
- The test case validates both initial calculation and updates with random data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Fisher Transform implementation exists
# Expected: Find Fisher.cs implementation and corresponding test file
# Check for Fisher implementation
fd -e cs Fisher lib/oscillators
# Check for Fisher tests
fd -e cs test.*oscillators Tests
Length of output: 130
Script:
#!/bin/bash
# Let's verify the actual implementation of Fisher Transform
cat lib/oscillators/Fisher.cs
# And check the test file content
cat Tests/test_updates_oscillators.cs
Length of output: 14033
Tests/test_updates_oscillators.cs (1)
328-329: Verify input type and period value for Fisher Transform.
The test uses TValue input type and period=10. Let's verify these choices:
✅ Verification successful
Fisher Transform implementation and test parameters are appropriate
The verification confirms that:
- The Fisher Transform implementation accepts a default period of 10, which matches the test's usage
- The indicator properly works with TValue input type as it inherits from AbstractBase
- The period=10 choice aligns with common practice for this indicator, as mentioned in the implementation's documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Fisher Transform implementation details
echo "Checking Fisher Transform implementation and common usage..."
# Look for Fisher class definition and constructor
ast-grep --pattern 'class Fisher {
$$$
Fisher($period) {
$$$
}
$$$
}'
# Search for documentation about recommended period values
rg -i "fisher.*period|period.*fisher" --type md
Length of output: 264
Script:
#!/bin/bash
# Let's try a broader search for Fisher class implementation
ast-grep --pattern 'class Fisher'
# Search for Fisher class files
fd -e cs -e fs Fisher
# Look for Fisher usage in tests
rg "new Fisher\(" -A 2
Length of output: 17450
Script:
#!/bin/bash
# Let's examine the Fisher class implementation
cat lib/oscillators/Fisher.cs
# Look for other Fisher indicator tests
rg "Fisher" Tests/ -A 5
Length of output: 3570
lib/oscillators/_list.md
Outdated
| @@ -1,5 +1,5 @@ | |||
| # Oscillators indicators | |||
| Done: 21, Todo: 8 | |||
| Done: 22, Todo: 7 | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix formatting in the count summary.
The counts are correctly updated to reflect the Fisher Transform completion. However, there's a minor formatting issue.
-Done: 22, Todo: 7
+Done: 22, To-do: 7📝 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.
| Done: 22, Todo: 7 | |
| Done: 22, To-do: 7 |
🧰 Tools
🪛 LanguageTool
[grammar] ~2-~2: It appears that a hyphen is missing in the noun “To-do” (= task) or did you mean the verb “to do”?
Context: # Oscillators indicators Done: 22, Todo: 7 ✔️ AC - Acceleration Oscillator ✔️ ...
(TO_DO_HYPHEN)
| public void Fisher_Update() | ||
| { | ||
| var indicator = new Fisher(period: 10); | ||
| double initialValue = indicator.Calc(new TValue(DateTime.Now, ReferenceValue, IsNew: true)); | ||
|
|
||
| for (int i = 0; i < RandomUpdates; i++) | ||
| { | ||
| indicator.Calc(new TValue(DateTime.Now, GetRandomDouble(), IsNew: false)); | ||
| } | ||
| double finalValue = indicator.Calc(new TValue(DateTime.Now, ReferenceValue, IsNew: false)); | ||
|
|
||
| Assert.Equal(initialValue, finalValue, precision); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation for the Fisher Transform test.
Consider adding a brief XML documentation comment explaining:
- The purpose of the Fisher Transform indicator
- Why period=10 was chosen as the test value
- Expected behavior with the reference value
Example:
/// <summary>
/// Verifies that the Fisher Transform indicator maintains consistency
/// when receiving repeated reference values after random updates.
/// Period of 10 is used as it's the commonly recommended value for this indicator.
/// </summary>
[Fact]
public void Fisher_Update()| for (int i = 0; i < RandomUpdates; i++) | ||
| { | ||
| indicator.Calc(new TValue(DateTime.Now, GetRandomDouble(), IsNew: false)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding edge case tests.
While the random update testing is good, consider adding specific test cases for:
- Extreme values (very high/low)
- Rapid value changes
- Constant values
Example addition:
// Test extreme values
indicator.Calc(new TValue(DateTime.Now, double.MaxValue * 0.1, IsNew: false));
indicator.Calc(new TValue(DateTime.Now, -double.MaxValue * 0.1, IsNew: false));
// Test constant values
for (int i = 0; i < 10; i++)
{
indicator.Calc(new TValue(DateTime.Now, 100.0, IsNew: false));
}| private readonly int _period; | ||
| private readonly double[] _prices; | ||
| private double _prevFisher; | ||
| private double _prevValue; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused field '_prevValue'
The field _prevValue is declared but never used in the class. Removing it will clean up the code and eliminate unnecessary clutter.
Apply this diff to remove the unused field:
private readonly int _period;
private readonly double[] _prices;
-private double _prevValue;
private double _prevFisher;📝 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.
| private double _prevValue; | |
| private readonly int _period; | |
| private readonly double[] _prices; | |
| private double _prevFisher; |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
[warning] 34-34:
The field 'Fisher._prevValue' is never used
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private double NormalizePrice(double price, double min, double max) | ||
| { | ||
| return 2 * ((price - min) / (max - min) - 0.5); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify arithmetic precedence with additional parentheses
Adding parentheses can make the arithmetic operations clearer and enhance code readability, even if the current expression is technically correct.
Apply this diff to add parentheses for clarity:
return 2 * ((price - min) / (max - min) - 0.5);
+return 2 * (((price - min) / (max - min)) - 0.5);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 66-66: lib/oscillators/Fisher.cs#L66
Arithmetic expressions should declare precedence. (SA1407)
Handle potential division by zero when 'max' equals 'min' in 'NormalizePrice'
In the NormalizePrice method, if max equals min, the denominator (max - min) becomes zero, leading to a division by zero error. This situation can occur when all prices in the period are the same.
Apply this diff to handle the scenario where max equals min:
private double NormalizePrice(double price, double min, double max)
{
+ var range = max - min;
+ if (range == 0)
+ return 0;
+ else
+ return 2 * ((price - min) / range - 0.5);
- return 2 * ((price - min) / (max - min) - 0.5);
}📝 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.
| return 2 * ((price - min) / (max - min) - 0.5); | |
| private double NormalizePrice(double price, double min, double max) | |
| { | |
| var range = max - min; | |
| if (range == 0) | |
| return 0; | |
| else | |
| return 2 * ((price - min) / range - 0.5); | |
| } |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 66-66: lib/oscillators/Fisher.cs#L66
Arithmetic expressions should declare precedence. (SA1407)
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private double FisherTransform(double value) | ||
| { | ||
| return 0.5 * System.Math.Log((1 + value) / (1 - value)); |
There was a problem hiding this comment.
Prevent division by zero or invalid logarithm in 'FisherTransform'
In the FisherTransform method, if value equals 1 or -1, the denominator (1 - value) becomes zero, and the logarithm function receives an undefined or infinite input. To prevent exceptions, clamp value to be within the open interval (-0.999, 0.999).
Apply this diff to clamp value within a safe range:
private double FisherTransform(double value)
{
+ const double limit = 0.999;
+ if (value >= limit) value = limit;
+ if (value <= -limit) value = -limit;
return 0.5 * System.Math.Log((1 + value) / (1 - value));
}📝 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.
| return 0.5 * System.Math.Log((1 + value) / (1 - value)); | |
| const double limit = 0.999; | |
| if (value >= limit) value = limit; | |
| if (value <= -limit) value = -limit; | |
| return 0.5 * System.Math.Log((1 + value) / (1 - value)); |
|
For more details, open the Copilot Workspace session.
Summary by CodeRabbit
Fisher_Updateto validate the functionality of the Fisher indicator.