-
Notifications
You must be signed in to change notification settings - Fork 25
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
Added moving average envelope issue #87 #115
Conversation
Seems there are no I think the name |
It was |
Oh, I just browsed the reference site again. They named it |
test/movingaverages.jl
Outdated
@test isapprox(upper.values[491] , 23.5587, atol=.01) | ||
@test isapprox(lower.values[1] , 88.9038, atol=.01) | ||
@test isapprox(lower.values[2] , 88.1838, atol=.01) | ||
@test isapprox(lower.values.values[490] , 19.1394, atol=.01) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo indeed.
src/movingaverages.jl
Outdated
push!(cname, string(cols[c], "_env_", n)) | ||
end | ||
|
||
TimeArray(tstamps, upper, cname, ta.meta), TimeArray(tstamps, lower, cname, ta.meta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to merge these two TimeArray?
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
==========================================
+ Coverage 93.75% 94.09% +0.34%
==========================================
Files 7 7
Lines 208 220 +12
==========================================
+ Hits 195 207 +12
Misses 13 13
Continue to review full report at Codecov.
|
src/movingaverages.jl
Outdated
upper = s + s * e | ||
lower = s - s * e | ||
|
||
upper, lower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about merging this also via [upper lower]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think [lower upper]
makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, okay.
src/movingaverages.jl
Outdated
s = sma(ta, n) | ||
|
||
upper = s.values .+ (s.values .* e) | ||
lower = s.values .- (s.values .* e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the broadcast of TimeArray works here.
upper = @. s + (s * e)
lower = @. s - (s * e)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works with s.values
.
src/movingaverages.jl
Outdated
cols = colnames(ta) | ||
for c in 1:length(cols) | ||
push!(cname, string(cols[c], "_env_", n)) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block can be turned into a broadcast, like:
julia> string.(colnames(ohlc), "_env_", 1)
4-element Array{String,1}:
"Open_env_1"
"High_env_1"
"Low_env_1"
"Close_env_1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contributions!
I've added moving average envelopes (
env
). Related tests seem to return anUndefVarError
even thoughenv
is exported insrc/MarketTechnicals.jl
. Any ideas?