-
Notifications
You must be signed in to change notification settings - Fork 69
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
allow views of timestamps in constructors #313
Conversation
Nice, thanks! Could you add a test for the new functionality? |
I have not used that style of test -- the old way the test would be e.g. items = 101:121
cols = 1:size(AAPL.values)[2]
tstamps = view(AAPL.timestamp, items)
tvalues = view(AAPL.values, items, cols)
APPL2 = TimeArray(tstamps, tvalues, AAPL.colnames, AAPL.meta)
@test AAPL[101:121] == AAPL2[1:end] |
test/timearray.jl
Outdated
tvalues = view(AAPL.values, items, cols) | ||
APPL2 = TimeArray(tstamps, tvalues, AAPL.colnames, AAPL.meta) | ||
|
||
@test AAPL[101:121] == AAPL2[1: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.
there is a macro @fact
from FactCheck...
Yes, as @iblis17 points out, this repository unfortunately still uses the outdated FactCheck package for its testing rather than the unit testing functionality provided in Base. To turn a |
I do not see the problem -- it works for me with cut and paste |
test/timearray.jl
Outdated
tstamps = view(AAPL.timestamp, source_rows) | ||
tvalues = view(AAPL.values, source_rows, source_cols) | ||
|
||
APPL2 = TimeArray(tstamps, tvalues, AAPL.colnames, AAPL.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.
ha, I got it. APPL2
is 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.
It should be AAPL2
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.
☕ very good noticing 🍇
test/timearray.jl
Outdated
facts("type constructors allow views") do | ||
|
||
source_rows = 101:121 | ||
source_cols = 1:size(AAPL.values)[2] |
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.
please purge the trailing whitespace :)
Looks great, thanks for the contribution! |
Should we move away from FactCheck? I guess this is probably an important issue. |
yes |
Yes, definitely. I believe it's no longer actively maintained, plus the Base unit testing framework is fantastic. |
In a general way -- the timestamp vector and the values array can be view()ed and the views used as args. |
@ararslan Here is the blocker I encountered: JuliaQuant/MarketTechnicals.jl#70 (comment) Our testing relies on tons of |
roughly is defined using isapprox # approx/roughly: Comparing numbers approximately
roughly(x::Number, atol) = (y::Number) -> isapprox(y, x, atol=atol)
roughly(x::Number; kvtols...) = (y::Number) -> isapprox(y, x; kvtols...)
roughly(A::AbstractArray, atol) = (B::AbstractArray) -> begin
size(A) != size(B) && return false
return isapprox(A, B, atol=atol)
end
roughly(A::AbstractArray; kvtols...) = (B::AbstractArray) -> begin
size(A) != size(B) && return false
return isapprox(A, B; kvtols...)
end As the tests use roughly, why not add the above into runtests.jl? |
allows TimeArrays to be constructed from view_s of another TimeArray's timestamps
just as TimeArrays can be constructed from view_s of another TimeArray's values