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

Fixed Minor Bugs inside the MA-functions #131

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

serkor1
Copy link
Contributor

@serkor1 serkor1 commented Feb 13, 2024

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:

  • HMA
  • ALMA
  • WMA
  • EVWMA

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!

serkor1 and others added 2 commits February 13, 2024 16:32
The following MA-functions didn't add a column name as the SMA-function does:

HMA
ALMA
WMA
EVWMA

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.
@joshuaulrich joshuaulrich merged commit 001c4da into joshuaulrich:master Feb 13, 2024
1 check passed
@joshuaulrich
Copy link
Owner

Thanks for the report and patch!

@joshuaulrich joshuaulrich added this to the [release candidate] milestone Feb 13, 2024
@ethanbsmith
Copy link
Contributor

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
Copy link
Owner

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?

joshuaulrich added a commit that referenced this pull request Feb 13, 2024
joshuaulrich added a commit that referenced this pull request Feb 13, 2024
@ethanbsmith
Copy link
Contributor

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
Copy link
Collaborator

braverock commented Feb 13, 2024

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
Copy link
Contributor

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

serkor1 added a commit to serkor1/cryptoQuotes that referenced this pull request Apr 25, 2024
All built-in data now only has the datasets keyword argument, which comes by default.

The chart_ma()-function has been rewritten to mitigate possible bugs. See for example joshuaulrich/TTR#131. The error is fixed, but its not published to CRAN yet. The new approach is robust against such bug-fixes for univariate series.

All moving average (MA) functions have been redocumented for usage and title sections.
@serkor1 serkor1 deleted the MA-Colnames-Fix branch April 25, 2024 20:14
serkor1 added a commit to serkor1/cryptoQuotes that referenced this pull request Apr 25, 2024
All built-in data now only has the datasets keyword argument, which comes by default.

The chart_ma()-function has been rewritten to mitigate possible bugs. See for example joshuaulrich/TTR#131. The error is fixed, but its not published to CRAN yet. The new approach is robust against such bug-fixes for univariate series.

All moving average (MA) functions have been redocumented for usage and title sections.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants