JuliaStats / TimeSeries.jl

Time series toolkit for Julia
Other
353 stars 69 forks source link

Symbol column indexing #377

Closed iblislin closed 5 years ago

iblislin commented 6 years ago

ref #262

codecov-io commented 6 years ago

Codecov Report

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

Impacted file tree graph

@@           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%) :arrow_down:
src/apply.jl 100% <100%> (ø) :arrow_up:
src/modify.jl 100% <100%> (ø) :arrow_up:
src/split.jl 100% <100%> (ø) :arrow_up:
src/readwrite.jl 100% <100%> (ø) :arrow_up:
src/utilities.jl 100% <100%> (ø) :arrow_up:
src/timearray.jl 98.03% <100%> (-0.2%) :arrow_down:
src/basemisc.jl 100% <100%> (ø) :arrow_up:
src/broadcast.jl 100% <100%> (ø) :arrow_up:

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 commented 6 years ago

What... the coverage is 99.x% :interrobang:

iblislin commented 6 years ago

ready for review.

femtotrader commented 6 years ago

LGTM

femtotrader commented 6 years ago

But doc should also be updated accordingly

femtotrader commented 6 years ago

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 commented 6 years ago

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

femtotrader commented 6 years ago

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!

iblislin commented 6 years ago

test cases and doc updated.

iblislin commented 6 years ago

Rebased. good to go?

ararslan commented 6 years ago

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

iblislin commented 6 years ago

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 commented 6 years ago

I decided to drop Memoize.jl finally