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

MACD Strategy doesn't consider crosses #365

Open
mozeryansky opened this issue Oct 21, 2023 · 2 comments
Open

MACD Strategy doesn't consider crosses #365

mozeryansky opened this issue Oct 21, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@mozeryansky
Copy link

Describe the bug
https://github.com/cinar/indicatorts/blob/main/src/strategy/trend/macdStrategy.ts

The strategy will say buy or sell based only on if the macd line is above or below the signal, and not when it crosses.

To Reproduce

import { macdStrategy } from 'indicatorts';
const asset: Asset = {
  dates: [],
  openings: [],
  highs: [],
  lows: [],
  closings: [
    19, 38, 55, 70, 83, 92, 98, 100, 98, 92, 83, 70, 55, 38, 19, 0, -19, -38,
    -55, -70, -83, -92, -98, -100, -98, -92, -83, -70, -55, -38, -19, 0, 19, 38,
    55, 70, 83, 92, 98, 100,
  ],
  volumes: [],
};
const actions = macdStrategy(asset);

Will produce: [0, -1, 1, -1, 1, 1, -1, -1, -1, 1, -1, 1, -1, -1, 1, -1, 1, -1, -1, 1, -1, -1, -1, -1, 1, 1, 1, -1, -1, 1, 1, 1, -1, 1, -1, -1, 1, -1, 1, -1]

Expected behavior

Only show buy/sell actions on the cross.

@cinar
Copy link
Owner

cinar commented Oct 21, 2023

Yes, this was bugging me for a while. Originally I had this based on the upward and downward movement when it crosses. Would be nice to make this change. I'll add it to the list, but if you want to fix it, please feel free to send a PR.

@cinar cinar added the bug Something isn't working label Oct 21, 2023
@mozeryansky
Copy link
Author

I had forked and was going to make the macd strategy take the periods as arguments, which is when I found the bug. The below change was what I quickly tried. But I believe there needs to be other considerations, as your macd/ema functions don't return NaN for the the values before the period begins, I believe that could cause false trade actions as well.

...
  for (let i = 1; i < actions.length; i++) {
    if (actions[i-1] != Action.BUY && macdResult.macdLine[i] > macdResult.signalLine[i]) {
      actions[i] = Action.BUY;
    } else if (actions[i-1] != Action.SELL && macdResult.macdLine[i] < macdResult.signalLine[i]) {
      actions[i] = Action.SELL;
    } else {
      actions[i] = Action.HOLD;
    }
  }
...

I've decided to use a market data API which returns technical indicators for now, but your code is really great and I'll return when the api I'm using reaches its usefulness for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants