search

joshuaulrich / TTR

Technical analysis and other functions to construct technical trading rules with R
GNU General Public License v2.0
326 stars 102 forks source link

Various minor fixes #80

Open jgehw opened 5 years ago

jgehw commented 5 years ago

For better "diff-ability" the following changes are separated into multiple commits:

  1. runSum/runMin/runMax have no dependency to n in cumulative case (parameter n is ignored and calculation is done for n=1) -> remove dependency to n for NA padding
  2. correct algorithmic errors resulting from copy+paste mistakes in implementation of stoch(), SMI(), and TRIX()
  3. fix growth function:
    • old implementation just returned series of NA because first value in cumprod was always NA due to na.pad in ROC() => set nad.pad=FALSE and pad with zero
    • ROC type must be discrete (not continuous) because we need factors (not differences) for use with cumprod
    • using factors we need 1/x values (not -x values) for sell signals
    • strip off last value from signals if signals has the same length as price because the outcome of the actions in signals is lagged by one (thus zero padding for first value == initial investment == 100%)
  4. add optional parameter accurate.instead.of.fast with default FALSE to runSum() function because if x contains a mixture of very large and small numbers, the fast version becomes inaccurate; this can lead to small negative running sums on all positive input data, which can lead to various secondary errors in other TTR functions (see 5. and 6.) built upon runSum
  5. add optional parameter accurate.instead.of.fast with default FALSE to SMA() and WMA() to be passed on to runSum()
  6. use accurate.instead.of.fast=TRUE in VWAP(), CMF(), DVI(), MFI(), rollSFM(), and volatility()
joshuaulrich commented 5 years ago

Thanks for your interest in contributing to TTR! I really appreciate the thought and effort you've put into this PR. That said, I wish you would have followed the contributing guide, and posted an issue first. That would have given me a chance to provide feedback before you spent time working on things that may not be merged.

I have several comments, but won't have time to do a comprehensive review until the weekend. It would really help me review if you could look at the commit messages section of the contributing guide and either amend your commit messages, or leave comments on each commit with an updated message (and I'll amend the commit messages).

jgehw commented 5 years ago

Sorry for not following the contributing guide. I amended the commit messages. Hopefully they now contain all required / helpful information. If something is still missing, please let me know.

joshuaulrich commented 5 years ago

Sorry for not following the contributing guide. I amended the commit messages. Hopefully they now contain all required / helpful information. If something is still missing, please let me know.

No need to apologize. The guide exists so contributors can know what I expect before they start work, so they don't spend their time working on something that is unlikely to be accepted. Thanks for the updated commit messages! Reviewing this PR is on my to-do list for this weekend.

jgehw commented 5 years ago

BTW: looking through existing issues, I noticed that this PR fixes issues 22, 45, and 74 :-)

joshuaulrich commented 5 years ago

Sorry it took me so long to get to this. I have some questions:

It would be great if you could provide unit tests for the following:

Please let me know if you're not willing/able to add unit tests. I'll add them before merging if you're not able to.

That's it. I think your PR looks great!

jgehw commented 3 years ago

Sorry for not replying any sooner (I was unable to work due to long-term sickness). Answering your questions:

Currently, I don't have time to add further unit tests, unfortunately. In case the commit messages are not sufficient for creating them, please feel free to ask.

The travis CI failed on macOS because of a package dependency error - doesn't seem to be related to the commits: * checking package dependencies ... ERROR Package required but not available: ‘zoo’

joshuaulrich commented 3 years ago

I need to get at TTR patch release to CRAN in the next couple days. This PR will be first on my review list after that release. Sorry it's languished this long.