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

add Indicator.unstablePeriod #918

Closed
wants to merge 22 commits into from
Closed

add Indicator.unstablePeriod #918

wants to merge 22 commits into from

Conversation

nimo23
Copy link
Contributor

@nimo23 nimo23 commented Nov 28, 2022

Fixes #.

Changes proposed in this pull request:

  • add Indicator#getUnstablePeriod()

  • added an entry with related ticket number(s) to the unreleased section of CHANGES.md

The main purpose of the PR is:

Actually, the user must know which value to set for Strategy#unstablePeriod. However, this is not easy to know, because the user is not always aware of the technical details of the indicators (e.g. SARIndicator needs at least 200 bars before the values can be calculated correctly, other indicators needs at least its barCount, etc.). It's better to have this information (about the unstablePeriod of an indicator) implemented and documented within each Indicator. The implementor of the indicator must provide this information within the source code of the indicator. With this, the user can retrieve this information (by Indicator#getUnstablePeriod()) for various use cases.

@nimo23
Copy link
Contributor Author

nimo23 commented Nov 29, 2022

Maybe we should also rename getUnstablePeriod() to getUnstableBars(), because we have bars and not periods, or?

@nimo23 nimo23 mentioned this pull request Feb 17, 2023
@team172011
Copy link
Member

I am still unsure about this enhancement, especially for indicators that have an unstable period, it should be up to the user to decide when the data of the indicator is useful. For example a user with 190 bars would like to use the SARIndicator in his strategy anyway.

Furthermore just adding the getUnstablePeriod() without any usages does not change anything nor? If introduced this concept must also be implemented in the BarSeriesManager where the unstablePeriod will be checked for each index

@nimo23
Copy link
Contributor Author

nimo23 commented Mar 19, 2023

I am still unsure about this enhancement, especially for indicators that have an unstable period, it should be up to the user to decide when the data of the indicator is useful.

I thought it should be up to the indicator (and not the user) when the indicator's data is correct. I also thought that if an indicator needs a "warm-up" period (=unstable period), then it is a fixed value that should at least be "documented" (= in the form of getUnstablePeriod() ) within our indicator implementations.

Furthermore just adding the getUnstablePeriod() without any usages does not change anything nor?

Yes, this PR is only about putting getUnstablePeriod() within our indicators, nothing more. The user can use this method for its use cases..

If introduced this concept must also be implemented in the BarSeriesManager where the unstablePeriod will be checked for each index

Yes good idea. We could do this in a new PR after this has been merged (as I also explained in #919 (comment)). But the intention of this PR is only to draw attention to the fact that indicators can have an unstable period and the user can get this automatically by calling a method (=getUnstablePeriod()). Without this method, the user does not know when the calculated indicator value is really correct and usable.

@nimo23
Copy link
Contributor Author

nimo23 commented Mar 20, 2023

Another thing: should we call it getUnstablePeriod() or getUnstableBars()? Which name is more appropriate?

@team172011
Copy link
Member

Another thing: should we call it getUnstablePeriod() or getUnstableBars()? Which name is more appropriate?

I would prefer getUnstableBars()

@nimo23 nimo23 marked this pull request as draft March 28, 2023 19:57
@nimo23
Copy link
Contributor Author

nimo23 commented Apr 1, 2023

Replaced by #944

@nimo23 nimo23 closed this Apr 1, 2023
@nimo23 nimo23 deleted the add_unstable_perdiod_to_Indicators branch April 13, 2023 21:00
@hhashim1
Copy link

Any updates/ETA on this?

@nimo23
Copy link
Contributor Author

nimo23 commented Apr 20, 2023

This PR is replaced by #944 (which is already merged). The issue #947 is the next step, feel free to share your suggestions in #947.

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.

None yet

4 participants