amv-dev / yata

Yet Another Technical Analysis library [for Rust]
Apache License 2.0
334 stars 52 forks source link

feat: Adding pivot point standard impls #37

Closed dandxy89 closed 10 months ago

dandxy89 commented 1 year ago

To implement this I needed to increase IndicatorResult result size to 12.

Resolves: https://github.com/amv-dev/yata/issues/36

dandxy89 commented 1 year ago

@amv-dev - can you take a look at this and advise on changes?

amv-dev commented 11 months ago

@dandxy89 Hello! Sorry for this looooooong anwer. I was having a lot of busy days... I don't think it is good idea to increase size for IndicatorResult. The whole idea of fixed size for IndicatorResult was totally wrong and I already working on next version of the library, where this issue is handled correctly.

But for now I'd recommend you to make you indicator as a Method, not an Indicator. Method allows to define it's own Output type, where all the signals may be described.

dandxy89 commented 11 months ago

No problem @amv-dev.

Thats for the feedback - much appreciated.

Let me re-familiarise myself with my changes and I'll update the PR over the next coming days.

dandxy89 commented 11 months ago

Just as an FYI this PR only contains a impl for Traditional.

Traditional <- Adding in this PR
Fibonacci
Woodie
Classic
DM
Camarilla

I can follow up with another PR to implement the remaining variants

dandxy89 commented 10 months ago

@amv-dev any you chance review this?

amv-dev commented 10 months ago

Please make this change and I will merge your PR.

But I think this feature must be refactored in the future. As I see, there is no internal state. On the other hand there are too many values in the output. So I think the whole feature might be created by traits. Something like

pub trait PivotPointStandard: OHLCV {
  fn pp(&self) -> ValueType {
    (self.high() + self.low() + self.close())/3.0
  }
  ...
}

impl<T: OHLCV> PivotPointStandard for T {}
dandxy89 commented 10 months ago

Thanks for the feedback.

I'll look at the suggestions later this year.