joshuaulrich / TTR

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

Fixed Minor Bugs inside the MA-functions #131

Closed serkor1 closed 4 months ago

serkor1 commented 4 months ago

Hi @joshuaulrich,

This is a somewhat trivial PR, I noticed a minor bug in some of your MA-functions which resulted in unexpected behavior; some of the MA-functions didn't add column names as expected. This is my attempt to fix it.

Here is my formatted commit-message:

The following MA-functions didn't add a column name as the SMA-function does:

The WMA were missing a reclass before the if-statement, and were therefore giving dim = NULL, which also affected the HMA. By fixing WMA, the ALMA and HMA got fixed as a by product by also adding an if-statement similar to the rest of the MA functions. The EVWMA function were wissing a reclass before the if-statement, so it never got evaluated inside the if-statement.

Please review the contributing guide before submitting your pull request. Please pay special attention to the pull request and commit message sections. Thanks for your contribution and interest in the project!

joshuaulrich commented 4 months ago

Thanks for the report and patch!

ethanbsmith commented 4 months ago

while i generally agree w/ this, just want to note that it is a breaking change and warrants a prominent callout. also, there are still other functions, like ROC that dont add a column name

joshuaulrich commented 4 months ago

Great point about it being a breaking change.

I don't have strong feelings about colnames for ROC() and momentum(). But I do think indicators should set column names. Currently at least one doesn't: williamsAD(). What do you think?

ethanbsmith commented 4 months ago

not sure i get the diff between the types of calculations ;) i often cbind the results of these calculations, so having a new name is generally useful (for me) consistency would probably be useful for new users

def in the realm of a lot of work for not a so much benefit, as anyone who cares about this is prolly already handling it themselves

braverock commented 4 months ago

I'm also generally in favor of setting column names.

A breaking change would be the change in the convention for setting column names, as some users might rely on the present convention for grep() or which() or similar. I don't think adding column names where none previously existed would be a breaking change though? For any existing code that sets column names (quantstrat does this if no column names exist), any existing use of colnames would still work.

ethanbsmith commented 4 months ago

i think basic xts (not sure where in the stack this is actually implemented) functionality will carry forward the existing column name unless you override it. so ROC(Cl(getSymbols("SPY"))) will have a column name of SPY.Close