Symbol column indexing #377

iblislin · 2018-10-06T14:03:57Z

add deprecation
support getproperty
support Base.propertynames
Update doc

ararslan · 2018-10-06T14:52:26Z

src/utilities.jl

+find_dupes_index(A::Vector{Symbol}) =
+    @inbounds [i for i in eachindex(A) if A[i] ∈ A[1:i-1]]
+
+@memoize function gen_colnames(n::Integer)


Have you seen a notable performance improvement when using @memoize?

yeah, this function is quite slow. (or my algo is bad, is there an better algo?

Orig:

julia> @btime TimeSeries.gen_colnames(5) 899.902 ns (9 allocations: 384 bytes) 5-element Array{Symbol,1}: :A :B :C :D :E

vs @memoize

julia> @btime TimeSeries.gen_colnames(5) 92.643 ns (2 allocations: 32 bytes) 5-element Array{Symbol,1}: :A :B :C :D :E

Interesting. I'll take a look at the algorithm itself in more detail later, this is fine in the meantime I suppose.

hmm...
How does DataFrames.jl generate the default column names?

IIRC it's just x appended with sequential numbers, i.e. x1, x2, ...

ararslan · 2018-10-07T05:31:24Z

test/combine.jl

+        @test_throws ErrorException merge(cl, op, colnames=[:a, :b, :c])
+
+        for mode ∈ [:inner, :left, :right, :outer]
+          @test merge(cl, ohlc[:High, :Low], mode, colnames=[:a, :b, :c]).colnames == [:a, :b, :c]


Should be the Julia standard 4 spaces for indentation

codecov-io · 2018-10-07T05:41:02Z

Codecov Report

Merging #377 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files           9        9           
  Lines         340      340           
=======================================
  Hits          337      337           
  Misses          3        3

Impacted Files	Coverage Δ
src/combine.jl	`98% <100%> (-0.44%)`	⬇️
src/apply.jl	`100% <100%> (ø)`	⬆️
src/modify.jl	`100% <100%> (ø)`	⬆️
src/split.jl	`100% <100%> (ø)`	⬆️
src/readwrite.jl	`100% <100%> (ø)`	⬆️
src/utilities.jl	`100% <100%> (ø)`	⬆️
src/timearray.jl	`98.03% <100%> (-0.2%)`	⬇️
src/basemisc.jl	`100% <100%> (ø)`	⬆️
src/broadcast.jl	`100% <100%> (ø)`	⬆️

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 5ece205...1afaadd. Read the comment docs.

iblislin · 2018-10-07T06:20:54Z

What... the coverage is 99.x% ⁉️

iblislin · 2018-10-07T07:59:49Z

ready for review.

femtotrader · 2018-10-07T11:29:25Z

LGTM

femtotrader · 2018-10-07T11:30:44Z

But doc should also be updated accordingly

femtotrader · 2018-10-07T11:33:23Z

You might search for " in

https://github.com/JuliaStats/TimeSeries.jl/blob/master/docs/src/indexing.md
https://github.com/JuliaStats/TimeSeries.jl/blob/master/docs/src/modify.md
https://github.com/JuliaStats/TimeSeries.jl/blob/master/docs/src/plotting.md
https://github.com/JuliaStats/TimeSeries.jl/blob/master/docs/src/split.md
https://github.com/JuliaStats/TimeSeries.jl/blob/master/docs/src/combine.md

iblislin · 2018-10-07T15:37:52Z

Ah, sure.
But I want to make another PRs for doc stuffs, including sort out docstring from current doc pages.

femtotrader · 2018-10-07T17:51:31Z

As API is changing (access column using Symbol instead of String) I think it's always safer to document changes and after (in an other PR) improve doc.
Because we don't know how long it will last to create the second PR... and during this delay doc and code will be inconsistent.
That's just my opinion. But code LGTM! You did a great work!

ararslan · 2018-10-07T23:52:30Z

src/timearray.jl

+    fs = fieldnames(T)
+
+    quote
+        (c ∈ $fs) ? getfield(ta, c) : ta[c]


If a user tries to call a column values, for example, this will be incredibly odd behavior. Either replace all field accesses within this package to use getfield explicitly, or don't support getproperty. (I'd vote for the latter.)

Okay, I need more time work on this issue.
There are tons of statements like ta.values in the test cases.

Yeah, every use of ta.values would need to be changed to getfield(ta, :values).

I will change them to our helper function vaules(ta), timestamp(ta), colnames(ta) and meta(ta).

ararslan · 2018-10-07T23:53:33Z

src/utilities.jl

+            if n == 1
+                cnames[d] = Symbol(cnames[d], "_$n")
+            else
+                s = cnames[d] |> string


Why change the function call to a pipe? It just makes it harder to read.

ararslan · 2018-10-07T23:53:49Z

src/utilities.jl

+findcol(ta::AbstractTimeSeries, s::Symbol) = findcol(colnames(ta), s)
+
+@inline function findcol(cols::Vector{Symbol}, s::Symbol)
+  i = findfirst(isequal(s), cols)


Incorrect indentation

test/timearray.jl

Since we are moving toward the symbol indexing. Ref JuliaStats/TimeSeries.jl#377

* Deprecate the helper functions Since we are moving toward the symbol indexing. Ref JuliaStats/TimeSeries.jl#377 * Update NEWS.md * travis: add 1.0 to build matrix - update doc deploy script

iblislin · 2018-10-13T06:26:05Z

test cases and doc updated.

iblislin · 2018-10-16T02:03:35Z

Rebased. good to go?

ararslan · 2018-10-16T16:43:40Z

docs/src/modify.md

-rename(cl, "New Close")
-rename(cl, ["New Close"])
-rename(ohlc, ["New Open", "New High", "New Low", "New Close"])
+rename(cl, :Close′)


Is the ' here intentional?

′ (not ') is used several times in https://github.com/JuliaStats/TimeSeries.jl/blob/master/src/broadcast.jl

I was surprised by the use of such a character also which can't be found easily on my AZERTY keyboard

Those render exactly the same for me. Weird.

Why not naming simply :NewClose (or :Close2)?

What are the english name of ′, ' and `

See screenshot to see how it's rendering here (France)

Not sure about the first one, the second is "apostrophe," and the third is "backtick" (assuming that's rendering for me properly).

It's :Close\prime<tab>

Why not naming simply :NewClose (or :Close2)?

well, I personally love the unicode support in Julia.
I always write code (in my research project) like x = 1; x\prime<tab> = x + 42
or
x\vec<tab> ...

julia> x = 1 1 julia> x′ = x + 42 43 julia> function ϕ() 42 end ϕ (generic function with 1 method) julia> :Close₂

Oh, I see. I was on Linux before when it rendered like an apostrophe for me, but on macOS they're distinct.

@ararslan maybe you should open an issue on your Linux distribution bug tracker because that's quite problematic to have 2 differents characters rendered the same.

ararslan · 2018-10-16T16:45:03Z

docs/src/timearray.md

+## Fields getter functions
+
+There are four field getter functions exported.
+They are named as same as the field names.


Field names are effectively an implementation detail, so I don't know that it's worth mentioning that. Good to document these functions though.

I think the field name is revealed to user already here:
http://juliastats.github.io/TimeSeries.jl/latest/timearray.html

So I added that section.

ararslan · 2018-10-16T16:46:18Z

src/combine.jl

+        colnames = Symbol.(colnames)
+    end
+
+    meta = if _meta(ta1) == _meta(ta2) && meta isa Nothing


meta === nothing is typically preferred to meta isa Nothing. In particular, === makes the compiler happy. (Though isa is still better than == in this case.)

ha, okay 5d1dcc7

ararslan · 2018-10-16T16:46:28Z

src/combine.jl

    else
-        meta = meta
+      meta


Indentation

ararslan · 2018-10-16T16:47:32Z

src/utilities.jl

+            if n == 1
+                cnames[d] = Symbol(cnames[d], "_$n")
+            else
+              s = string(cnames[d])


Indentation

ararslan · 2018-10-16T16:48:55Z

Looks good to me for the most part, just a few comments. Nice work here!

iblislin · 2018-10-17T02:53:00Z

One more thing to be discussed:
This warning will pop up during recompilation. Is it safe to ignore it ?

julia> using TimeSeries
[ Info: Recompiling stale cache file /home/iblis/.julia/compiled/v1.0/TimeSeries/dmqCl.ji for TimeSeries [9e3dc215-6440-5c97-bce1-76c03772f85e]
WARNING: eval from module Memoize to TimeSeries:
Expr(:const, Symbol("##gen_colnames_memoized_cache") = Expr(:call, :IdDict))
  ** incremental compilation may be broken for this module **

WARNING: eval from module Memoize to TimeSeries:
Expr(:const, Symbol("##carry_char_memoized_cache") = Expr(:call, :IdDict))
  ** incremental compilation may be broken for this module **

iblislin · 2018-10-20T06:19:45Z

I decided to drop Memoize.jl finally

ararslan reviewed Oct 6, 2018

View reviewed changes

ararslan reviewed Oct 7, 2018

View reviewed changes

iblislin force-pushed the ib/symbol branch from a4b6f5f to 74ea235 Compare October 7, 2018 06:15

iblislin added the need news label Oct 7, 2018

iblislin changed the title ~~WIP: Symbol column indexing~~ Symbol column indexing Oct 7, 2018

ararslan reviewed Oct 7, 2018

View reviewed changes

test/timearray.jl Show resolved Hide resolved

femtotrader mentioned this pull request Oct 8, 2018

Symbol indexing femtotrader/TimeSeriesResampler.jl#26

Closed

iblislin added a commit to JuliaQuant/MarketData.jl that referenced this pull request Oct 9, 2018

Deprecate the helper functions

50ab74f

Since we are moving toward the symbol indexing. Ref JuliaStats/TimeSeries.jl#377

iblislin mentioned this pull request Oct 9, 2018

Deprecate the helper functions JuliaQuant/MarketData.jl#51

Merged

iblislin added 8 commits October 16, 2018 10:03

start working on Symbol column indexing

d847747

getindex

02925a4

deprecation

a2d3a01

fix readwrite.jl

d39f56d

modify: rename

b96edec

apply: uniformspace

eac66e2

fix broadcast

a222f08

fix replace_dupes

1ab8c56

iblislin added 11 commits October 16, 2018 10:03

doc: split.md

132e940

doc: modify.md

fb1adb8

doc: apply.md

3796359

doc: combine.md

c7fbf6b

doc: plotting.md

6f67f84

doc: dotfile.md

70f5a2b

doc: readwrite.md

e1b29e3

fix update ohlc

48465b9

style stuff

47363b8

fix update function

670ec9b

doc/dotfile.md: correct edit command

17fef95

iblislin force-pushed the ib/symbol branch from 53699e6 to 17fef95 Compare October 16, 2018 02:03

ararslan reviewed Oct 16, 2018

View reviewed changes

iblislin added 2 commits October 17, 2018 10:34

using === (\euqiv) in case of singleton

5d1dcc7

fix indentation

9262ce7

iblislin force-pushed the ib/symbol branch from b430429 to bb2d6c6 Compare October 20, 2018 06:29

drop Memoize

1afaadd

iblislin force-pushed the ib/symbol branch from bb2d6c6 to 1afaadd Compare October 20, 2018 06:52

update NEWS

1987ea9

iblislin merged commit b98c968 into master Oct 24, 2018

iblislin deleted the ib/symbol branch October 24, 2018 02:05

iblislin removed the need news label Oct 24, 2018

Symbol column indexing #377

Symbol column indexing #377

Conversation

iblislin commented Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

iblislin Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 7, 2018 • edited Loading

Codecov Report

iblislin commented Oct 7, 2018

iblislin commented Oct 7, 2018

femtotrader commented Oct 7, 2018

femtotrader commented Oct 7, 2018

femtotrader commented Oct 7, 2018

iblislin commented Oct 7, 2018

femtotrader commented Oct 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iblislin Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iblislin commented Oct 13, 2018

iblislin commented Oct 16, 2018

Choose a reason for hiding this comment

femtotrader Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iblislin Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Oct 16, 2018

iblislin commented Oct 17, 2018

iblislin commented Oct 20, 2018

iblislin commented Oct 6, 2018 •

edited

Loading

iblislin Oct 6, 2018 •

edited

Loading

codecov-io commented Oct 7, 2018 •

edited

Loading

iblislin Oct 9, 2018 •

edited

Loading

femtotrader Oct 16, 2018 •

edited

Loading

iblislin Oct 17, 2018 •

edited

Loading