Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement strategies #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement strategies #1

wants to merge 2 commits into from

Conversation

hardvain
Copy link
Owner

@hardvain hardvain commented Dec 30, 2020

This PR extracts signals from indicators and introduces strategies to capture signals

The build at the moment fails because not all indicators are updated to return only values

#[derive(Clone, Copy)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct IndicatorResult {
signals: [Action; IndicatorResult::SIZE],
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing signals from Indicator result

}

impl IndicatorResult {
/// Size of pre-allocated result array
/// For the most of cases it should not be used anywhere outside this crate
pub const SIZE: usize = 4;

/// Returns a slice of signals of current indicator result
#[must_use]
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing all signal related methods

@@ -0,0 +1,131 @@
use crate::core::{ValueType, IndicatorResult, Action, Error, OHLCV};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contains all strategy related traits. Mostly copied from indicator traits. Can be optimized

#[inline]
#[must_use]
pub fn value(&self, index: usize) -> ValueType {
self.indicator_result.value(index)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delegate to indicator value.

@@ -0,0 +1,56 @@
use crate::core::{PeriodType, StrategyConfig, OHLCV, Error, StrategyInstance, IndicatorConfig, Source, StrategyResult, Action, IndicatorInstanceDyn};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A composite strategy that uses HMA & Pivot Reversal indicators

@@ -0,0 +1,55 @@
use crate::core::{PeriodType, StrategyConfig, OHLCV, Error, StrategyInstance, IndicatorConfig, Source, StrategyResult, Action, IndicatorInstanceDyn};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A composite strategy that uses HMA & EMA indicators

@amv-dev
Copy link

amv-dev commented Jan 2, 2021

I don't see any reason for removing signals from the indicators. Indicators values itself doesn't make sense. We always need to interpret it somehow.
For common indicators this interpretation is pretty fixed. So there is no reason to remove signals. If someone doesn't want a regular values interpretation, he can always wrap indicator and interpret raw values by himself. But in most cases this will not happen.

On the other hand, strategies should receive indicators signals and then proceed a final action for trader. That might be not only simple "Sell" or "Buy", but something more complex like "stop limit" or "trailing stop", or something else.

@amv-dev
Copy link

amv-dev commented Jan 2, 2021

Ok, I think a little bit more about your idea and changed my decision. According to my benchmarks processing signals may take up to 50% of the indicator's performance. So, yes, if you as developer don't like current signals implementation of some indicator, you starting to waste performance to calculate those useless signals.
I need time to think about this idea. This change will not happen in v0.4 as long as it will brake compatibility. So if I decide to apply your suggestion, I will create a separate branch for this changes.

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.

2 participants