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

feat: Adding pivot point standard impls #37

Closed
wants to merge 3 commits into from
Closed

feat: Adding pivot point standard impls #37

wants to merge 3 commits into from

Conversation

dandxy89
Copy link
Contributor

@dandxy89 dandxy89 commented Nov 4, 2023

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

Resolves: #36

@dandxy89
Copy link
Contributor Author

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

@amv-dev
Copy link
Owner

amv-dev commented Jan 8, 2024

@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
Copy link
Contributor Author

dandxy89 commented Jan 8, 2024

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
Copy link
Contributor Author

dandxy89 commented Jan 9, 2024

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 dandxy89 requested a review from amv-dev January 22, 2024 11:41
@dandxy89
Copy link
Contributor Author

dandxy89 commented Feb 2, 2024

@amv-dev any you chance review this?

///
/// * <https://en.wikipedia.org/wiki/Pivot_point_(technical_analysis)>
///
pub struct PivotPointStandard {}
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove those curly brackets. It's useless.

@amv-dev
Copy link
Owner

amv-dev commented Feb 3, 2024

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 dandxy89 closed this Feb 3, 2024
@dandxy89 dandxy89 deleted the feat/pivot-point branch February 3, 2024 08:49
@dandxy89
Copy link
Contributor Author

dandxy89 commented Feb 3, 2024

Thanks for the feedback.

I'll look at the suggestions later this year.

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.

Feat: Adding Pivot Points Standard impls from TradingView
2 participants