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

Add a typecast to fit the type b/w sma and runmean #25

Closed
wants to merge 1 commit into from

Conversation

hassiweb
Copy link

Due to the mismatch of the argument types between sma and runmean, it should be typecasted from Real to Float.

Due to the mismatch of the argument types between `sma` and `runmean`, it should be typecasted from Real to Float.
@codecov
Copy link

codecov bot commented Apr 12, 2020

Codecov Report

Merging #25 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #25   +/-   ##
=======================================
  Coverage   95.40%   95.40%           
=======================================
  Files          11       11           
  Lines         806      806           
=======================================
  Hits          769      769           
  Misses         37       37           
Impacted Files Coverage Δ
src/ma.jl 97.36% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34704f2...c69c38a. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.409% when pulling c69c38a on hassiweb:fix-type-mismatch into 34704f2 on dysonance:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.409% when pulling c69c38a on hassiweb:fix-type-mismatch into 34704f2 on dysonance:master.

@dysonance
Copy link
Owner

@hassiweb Thanks for contributing and opening the pull request here. I'm not sure I understand what's causing the issue here? Could you provide some context around what the problem is so I can understand? The diff looks okay, I just don't understand why it's necessary to do this conversion for this specific pair of functions vs. the others?

@hassiweb
Copy link
Author

@dysonance Thanks for your comment. I faced this issue when I use the sma function with a Real-type array variable. The argument of sma is defined as Real-type, but the argument of runmean used in sma is defined as Float64. Due to this type-mismatch, it failed without casting the variable into Float64.
After I made this PR, actually I found that other functions have the same issue, such as macd. How do you think which is better to change all functions that use runmean or to change runmean itself? Actually, I'm a newbie of Julia. I'm not familiar with the performance in terms of the difference of the variable types.

@dysonance
Copy link
Owner

@hassiweb This has been addressed as of the latest commits on master. I'm working on pushing out a release as we speak, so I'm going to go ahead and close this now.

@dysonance dysonance closed this Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants