Indicators use RegularMethods instead of Method

[raimundomartins]

Most Indicators use the RegularMethods type. For me to use an existing indicator with a new Method I make I have to clone the source and add the new method to RegularMethods. Why don't they just use a Method?

Related to this is that by using the RegularMethods you are forcing the usage of Box and possible multiple-calculation of the same thing. If I want to use MACD and Keltner Channels in which one of the methods and its period is the same we are making double calculations. If Indicators take instead &'a dyn Method we would be both generalizing and allowing for more optimizations.

[amv-dev]

Most Indicators use the RegularMethods type. For me to use an existing indicator with a new Method I make I have to clone the source and add the new method to RegularMethods. Why don't they just use a Method?

Yes, it is a real issue for now. But there were bunch of reasons and decisions behind RegularMethods type.

First of all, we can't just use dyn Method, neither immutable, nor mutable because we lose all invariants between whole indicator and method it uses. All the parts inside Indicator must be synchronized. There is a chance that programmer can provide non-sychronized method into Indicator's config. Something like this:

let mut candles = yata::helpers::RandomCandles::default(); // just some candles data

let mut ma1 = EMA::new(5, candles.next().close()); // fast MA

let mut ma2 = EMA::new(10, candles.next().close()); // slow MA
ma2.next(candles.next().close()); // ma2 is totally desynced with ma1

let mut macd = MACD::default();
macd.ma1 = &mut ma1;
macd.ma2 = &mut ma2;
// Now `macd` has totally wrong state inside

Second reason is that sometimes we need one method result to initialize another method inside indicator. Therefore we can't just assign method instance to an indicator's config. For example, MACD has three methods inside. Two of them use original data, but third is based upon other two methods results. So we can't just assign method3 to MACD with &dyn Method.

There are some other reasons for RegularMethods to be part of this library for now. It might be not a good solution, but I don't know better one for now.

Related to this is that by using the RegularMethods you are forcing the usage of Box and possible multiple-calculation of the same thing. If I want to use MACD and Keltner Channels in which one of the methods and its period is the same we are making double calculations. If Indicators take instead &'a dyn Method we would be both generalizing and allowing for more optimizations.

It is not possible even with &dyn Method. Every time you call next on any Method you mutate it's state. So if you want provide single method references to several indicators, you run into big issue with Rust rules of mutability. Both indicators must have mutable reference to method's instance. This is where Rust really helps to avoid mistakes because every time first indicator uses method it modifies it's state. So second indicator will have different state of the same method.

[raimundomartins]

All the parts inside Indicator must be synchronized.

Perhaps in some cases, but who's to say I don't want to try out something out-of-the-box and delay the slow MA by one candle (the opposite of what you suggested)?
For me the whole idea of using Rust and not some tradeview script is the liberty to mess around all I want as well as reuse code later for some bot.

Second reason is that sometimes we need one method result to initialize another method inside indicator.

This is a good point, indeed. But the whole initialization feels wrong as it is anyway. I've read that the recommended initial value of EMA is SMA, yet you use the first candle. Plus for MACD, if I choose E/D/TMA it takes much more than period3 periods for the "wrong" initial value of the first candle to fully dissipate.
Anyway, for this particular issue, MACD could be generic over a Method<Input = PeriodType, Params = ValueType> like every RegularMethod is, essentially removing the Box. It could even store the Method directly in its fields, instead of a reference.
Have you ever considered using generics? (honest question, not a confrontational one)

It is not possible even with &dyn Method.

Yes, this occured to me only a while after I opened the issue. It would require some complex stuff to make this work for probably too little benefit. Withdrawn...

[amv-dev]

the whole initialization feels wrong as it is anyway. I've read that the recommended initial value of EMA is SMA

It is useless recommendation. Just ask yourself: if initial value of EMA is SMA, then what initial value of that SMA?

Initialization of methods and indicators may depend, but this library aims to follow next invariant:

• If you provide constant input value since the beginning, then output value must not change (except rounding error in IEEE 754);

It's like you have "virtual" infinite previous history with constant value.

if I choose E/D/TMA it takes much more than period3 periods for the "wrong" initial value of the first candle to fully dissipate

Exponential moving averages uses all the history it has. There is no such a period, after you can tell "Now it's totally correct". So, again, the same invariant of infinite previous history helps a lot.

MACD could be generic over a Method<Input = PeriodType, Params = ValueType> like every RegularMethod is, essentially removing the Box. It could even store the Method directly in its fields, instead of a reference.

Well... It can... But MACD has three different moving averages. So there must be three different generic parameters for MACD:

struct<M1, M2, M3> MACDInstance 
where M1: Method<Params = ValueType, Input = PeriodType, Output = ValueType>,
M2: Method<Params = ValueType, Input = PeriodType, Output = ValueType>
M3: Method<Params = ValueType, Input = PeriodType, Output = ValueType>
{
// ...
}

It may become a generic hell. There are bunch of other negative side effects of this architecture. F.e. you can't no longer return IndicatorInstance from IndicatorConfig, because of that generic parameters.

[raimundomartins]

It is useless recommendation. Just ask yourself: if initial value of EMA is SMA, then what initial value of that SMA?

Ah, that one is easy and the the core of why initialization feels off to me: it is not initialized until it gets period inputs.

Exponential moving averages uses all the history it has.

Mathematically, yes, but in practice eventually far away periods become meaningless and even get lost in the floating point precision, as I'm sure you're aware.

you can't no longer return IndicatorInstance from IndicatorConfig, because of that generic parameters.

I fail to understand this. You would add verbosity to implementors of IndicatorConfig but it wouldn't be impossible.
I think we just have different views on the subject of generics and there are obviously cons and pros for both approaches, so I'll stop pushing that button.

But I'd really love if your crate were more open to extensability! As I see it, this mainly requires separating configuration from initialization (like not assuming Method is ready on a single value). In this particular case, EMA would take the number of periods as config, but MACD would give the initial value. You would then be able to send any &dyn Method. I don't know all of your codebase, so maybe there are reasons not to do this.

Note that my issue is only against enum RegularMethods not type RegularMethod (even if it has a Box which I don't enjoy :P)

[amv-dev]

As I see it, this mainly requires separating configuration from initialization (like not assuming Method is ready on a single value). In this particular case, EMA would take the number of periods as config, but MACD would give the initial value.

I thought about this issue and came to the same decision. So my proposal is to make a trait MovingAverageConfig like this:

pub trait MovingAverageConfig {
  fn init(&self, initial_value: ValueType) -> Result<Box<dyn Method<Params = PeriodType, Input = ValueType, Output = ValueType>>, Error>;
}

Then create enum similar to RegularMethods which will implement MovingAverageConfig:

pub enum MovingAverages {
   SMA(PeriodType),
   WMA(PeriodType,
  // ...
}

impl MovingAverageConfig for MovingAverages {
  fn init(&self, initial_value: ValueType) -> Result<Box<dyn Method<Params = PeriodType, Input = ValueType, Output = ValueType>>, Error> {
    match self {
      Self::SMA(length) => Ok(Box::new(SMA::new(length, initial_value)?),
      Self::WMA(length) => Ok(Box::new(WMA::new(length, initial_value)?),
      // ...
    }
  }
}

And indicators will be generalized over M:

pub struct MACD<M: MovingAverageConfig> {
  // ...
}

On one hand it is pretty the same as it's implemented now. On the other hand, if you need to add your own moving average, you just need to create your own enum (or any other type) and implement MovingAverageConfig for it. Then you can use your own moving averages constructor in any indicator.

[raimundomartins]

Yes, I think something like this is much better! I'm assuming MACD now takes 3 MovingAverageConfig instances, right?

Maybe with a macro it gets simple to even impl MovingAverageConfig for *MA to use in other indicators. Also, I don't know if MovingAverage is the best name as this may apply to other methods. Maybe RegularMethodConfig is better?

[amv-dev]

I've created a new branch. There are lot of changes, but the most important are:

• Created marker trait MovingAverage as a supertrait of Method<Params = PeriodType, Input = ValueType, Output = ValueType>;
• Created trait MovingAverageConstructor;
• Created enum MA which implements MovingAverageConstructor;
• Indicator configs, which can can use different moving averages, are now generic over M: MovingAverageConstructor.

method, RegularMethod and RegularMethods are totally removed.

So if you want to use your own moving average, you need to create some type (probably just an enum) and implement MovingAverageConstructor for it. This is still WIP, but you can try it.

[amv-dev]

Resolved in v0.5