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

MACD implementation not correct? #152

Closed
cykedev opened this issue Jan 14, 2014 · 8 comments
Closed

MACD implementation not correct? #152

cykedev opened this issue Jan 14, 2014 · 8 comments

Comments

@cykedev
Copy link
Contributor

cykedev commented Jan 14, 2014

Is your MACD implementation correct?
In moving-average-convergence-divergence.js you do the following:

calculate the EMAs

TradingMethod.prototype.calculateEMAs = function (candle) {
  // the two EMAs
  _.each(['short', 'long'], function (type) {
    this.ema[type].update(candle.c);
  }, this);
  // the MACD
  this.calculateEMAdiff();
  // the signal
  this.ema['signal'].update(this.diff);
}

Calculate the diff

TradingMethod.prototype.calculateEMAdiff = function () {
  var shortEMA = this.ema.short.result;
  var longEMA = this.ema.long.result;

  this.diff = 100 * (shortEMA - longEMA) / ((shortEMA + longEMA) / 2);
}

According to Wikipedia this should be something like that:

TradingMethod.prototype.calculateEMAdiff = function () {
  var shortEMA = this.ema.short.result;
  var longEMA = this.ema.long.result;

  this.diff = shortEMA - longEMA;
}

After that the calculateAdvice method should use the correct values internal.
Correct me if if I'm wrong :-)

@cykedev
Copy link
Contributor Author

cykedev commented Jan 14, 2014

MACD is an absolute price oscillator, so the threshold in the configuration makes no sense. The filterting has to be done with the duration of the trend.

@cykedev
Copy link
Contributor Author

cykedev commented Jan 14, 2014

Maybe better than MACD could be PPO which shows the MACD as percentage values. The current algorithm looks like that ...

The Calculation of PPO would be:

Percentage Price Oscillator (PPO): {(12-day EMA - 26-day EMA)/26-day EMA} x 100
Signal Line: 9-day EMA of PPO
PPO Histogram: PPO - Signal Line

I think thats what you wanted... and the threshold makes sense again

Quote from stockcharts

Conclusions

The Percentage Price Oscillator (PPO) generates the same signals at MACD, but provides an added dimension as a
percentage version of MACD. The PPO levels of the Dow Industrials (price ~11000) can be compared against the PPO
levels of IBM (price ~122) because the PPO levels the playing field so to speak. In addition, PPO levels in one 
security can be compared over extended periods of time, even if the price has doubled or tripled. This is not the case
for MACD. Despite its advantages, PPO is still not the best oscillator to identify overbought or oversold conditions 
because movements are unlimited (in theory). Levels for RSI and the Stochastic Oscillator are limited and this makes
them better suited to identify overbought and oversold levels.

My test:

TradingMethod.prototype.calculateEMAdiff = function () {
  var shortEMA = this.ema.short.result;
  var longEMA = this.ema.long.result;

  this.diff = 100 * ((shortEMA - longEMA) / longEMA);
}

@askmike
Copy link
Owner

askmike commented Jan 14, 2014

@cykedev thanks for raising this issue. The MACD addition was a pull request and for reasons such as this one I consider this branch to be very unstable. Verifying the MACD was on still on my todo so thanks for helping me out here.

As for the wrong data: I'll look into it right now but I'm pretty sure you are right.

MACD is an absolute price oscillator, so the threshold in the configuration makes no sense. The filterting has to be done with the duration of the trend.

Not quite sure why that would make the threshold obsolete (I still have to do more research as well). Code wise this method is based on the DEMA method (just the diff of 2 different EMAs). There the threshold indicate the weight of the trend (ie: we only call it an uptrend if it's going up more than X), this check is applied after the EMA calculation and only effects the decision making and not the raw EMA calculation.

I am currently in the process of abstracting the indicator logic from the trading decision: I want an DEMA method (the one described above) and a MACD indicator that you feed data and it will spit out a number indicating the discovered trend (so the number we are now using in the advice method). This way the trading method looks simpler by just holding the logic that deals with the result of the trend calculation and we can also do stuff like use both DEMA, MACD, RSI, etc. and having the decision logic completely decoupled from these raw calculations.

I'll try your proposed solution and I'll also create a test suite to verify results against another MACD calculation tool (like this one). I'm also very open to pull requests!

@djmuk
Copy link
Contributor

djmuk commented Jan 14, 2014

Yes - technically I have implemented it as a PPO not an absolute one. This made sense to me because the existing EMA method also uses a percentage diff calculation not an absolute one and it is 'easier' when dealing with multiple exchange pairs that may have wildly varying prices to compare them using percentage thresholds rather than absolute ones, and once a set of values that 'work' have been found on one exchange pair then they can easily be trasnferred to another pair.

I don't understand your issue with the threshold - if you want a 'true' zero crossing then just set it to a really small value (and deal with the false crossings), similarly with the duration value - set it to 1.

@Removed-5an
Copy link
Contributor

@cykedev Even if MACD is an absolute oscillator, I still like thresholds. It gives you more options while not compromising the original indicator. If you don't like thresholds, just set both as 0.

@Removed-5an
Copy link
Contributor

Fixed in pull request:

#155

PPO Should be an entirely different indicator.

@cykedev
Copy link
Contributor Author

cykedev commented Jan 14, 2014

@askmike @djmuk don't get me wrong - I was not sure if the PPO implementation was intended. In my opinion PPO is better - it makes the values comparable.

I just thought that MACD with absolute values can not properly use the threshold - what if price rises from $10 to $1000; the same config value would cause different behavior.

But with PPO it's great! But shouldn't it be

this.diff = 100 * ((shortEMA - longEMA) / longEMA);

? What is the difference to dividing by the average of short and long?

@5an1ty according the pull request - what is intended in this MACD implementation - absolute or relative with PPO? Maybe we could include a config value and switch the behavior in the implementation; MACD and PPO are not completely different, we could save some duplicated code...

@Removed-5an
Copy link
Contributor

@cykedev the pull request I submitted is to achieve the traditional MACD indicator with an absolute diff.

While I agree code duplication is a bad thing, I look at indicators as plugins and for plugins I rate code separation above code deduplication, so imho we should keep it as two different indicators.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants