Skip to content
This repository has been archived by the owner on Feb 16, 2020. It is now read-only.

Add SMMA, replace RSI with original study #925

Merged
merged 5 commits into from
Aug 14, 2017
Merged

Conversation

wuhkuh
Copy link
Contributor

@wuhkuh wuhkuh commented Aug 6, 2017

Part 1/2 of #759

I'm going to split the pull request as this one is backwards compatible. The next part is not - it will replace the current RSI function with the one from the original study.

This means the patch version can be incremented for this specific PR, instead of a semver-minor.
Tests are included, however, it looked like the test suite is broken for this repository.
Anyway, I just added them in the same style as the tests for the other indicators.

The pre-calculated results are taken from my own repository, which is documented and tested thoroughly. Sources used to determine the correct algorithm are documented, as well.

@askmike
Copy link
Owner

askmike commented Aug 7, 2017

Thanks a lot! Great stuff, let me know when you have both parts ready and I'll go over them.

@wuhkuh wuhkuh changed the title Add Smoothed Moving Average (SMMA) Add SMMA, replace RSI with original study Aug 7, 2017
@wuhkuh
Copy link
Contributor Author

wuhkuh commented Aug 7, 2017

Part 2/2 of #759

RSI has been replaced with the original study. Once again, tests are included and verified.
I haven't changed the interpretation of the RSI value, located under strategies/RSI.js.

Keep in mind that this can will break the setup of the users.

I'd suggest doing a minor increment when implementing this, for this reason.

I assumed you'd want both parts together, so I put them together in this PR.
If not, I can split the pull request and send them in separately.

@askmike
Copy link
Owner

askmike commented Aug 14, 2017

This PR looks great, thanks a lot for the time you put in :)

Keep in mind that this can will break the setup of the users.

What exactly will this break besides calculating different results?

@wuhkuh
Copy link
Contributor Author

wuhkuh commented Aug 14, 2017

What exactly will this break besides calculating different results?

This is exactly it. I used the term break just to clarify that this change isn't a simple fix. I should've worded it differently, you're right.

Users of Gekko are probably used to working with the current results and I can imagine they wouldn't be too happy if we lower their revenue by implementing this change.

Assuming you're using Semver, if this version were to be 1.0.0 or higher, this change would classify as a semver-major - because we will change users results.
However, we aren't past 1.0.0 - so I'd suggest merging this whenever you're ready for 0.6.0.

@askmike
Copy link
Owner

askmike commented Aug 14, 2017

so I'd suggest merging this whenever you're ready for 0.6.0.

Great, I will make it part of the release described in #911 and merge it into develop (the nightly branch now).


Assuming you're using Semver, if this version were to be 1.0.0 or higher, this change would classify as a semver-major - because we will change users results.

Semver describes versioning according to API changes, but since Gekko is not a library that exposes a single API I found semver hard to follow:

  • Gekko (CLI) can be configured using a config, the form of which has changed quite drastically.
  • Gekko (UI) can also be configured (hosts, ports, etc), this config format has also changed.
  • Gekko writes files to disk, very rarely I had to update this format (causing the need for users to migrate data)
  • Gekko exposes a REST API (which is undergoing a lot of changes).
  • The functionality available to strats (the "strat API") (which this PR does not change) changes sometimes.
  • Functionality changes (new exchanges, new markets, new functionality).

For that reason I found it impossible to follow strict semver since all the above can be argued to be "APIs", and a lot of them have had breaking changes in the past. Also I am not comfortable labeling this v1+.

@askmike askmike changed the base branch from stable to develop August 14, 2017 18:22
@askmike askmike merged commit 3ef5d26 into askmike:develop Aug 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants