dysonance / Temporal.jl

Time series implementation for the Julia language focused on efficiency and flexibility
Other
101 stars 27 forks source link

rename AdjClose column #31

Closed femtotrader closed 4 years ago

femtotrader commented 6 years ago

Closes #20

dysonance commented 6 years ago

Thanks for the fix here @femtotrader. The only thing that makes me a little cautious about this is having inconsistent behavior the column naming convention. Having the Yahoo data downloads apply such a specific change begs the question of how to treat other datasets that also include spaces in the field names. What if someone downloads a Quandl time series that has spaces in the names. They would encounter the same ultimate issue but it wouldn't have this specific treatment.

A related concern was brought up a while ago about changing field names by default. Eliminating whitespace and non-alphanumeric characters used to be the default behavior, but this issues raised an interesting point that enforcing some default behavior may not be best practice.

Since this issue ultimately boils down to issues with the plotting interface, I wonder if it would be better to make the changes there. That is, alter the column names before generating the plot so the legends are sensible. What do you think?

femtotrader commented 6 years ago

Hope to you!

Maybe a more general approach could be to do here (and probably for some other remote sources)

function sanitize_colname(s::String)
    replace(s, " " => "")
    replace(s, "-" => "")
    replace(s, "." => "")
    s
end

rename!(sanitize_colname, data, String)

but anyway I don't think sanitizing by default column names when constructing TS was a great idea.

I think it should only be done when necessary (and the sanitize_colname could be different depending of the remote source)

codecov[bot] commented 6 years ago

Codecov Report

Merging #31 into master will decrease coverage by 30.67%. The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #31       +/-   ##
===========================================
- Coverage   87.41%   56.74%   -30.68%     
===========================================
  Files          13       14        +1     
  Lines         620      964      +344     
===========================================
+ Hits          542      547        +5     
- Misses         78      417      +339
Impacted Files Coverage Δ
src/io.jl 12.69% <0%> (-71.52%) :arrow_down:
src/models.jl 50% <0%> (-35%) :arrow_down:
src/collapse.jl 64.78% <0%> (-33.09%) :arrow_down:
src/ts.jl 70.37% <0%> (-29.63%) :arrow_down:
src/utils.jl 55.17% <0%> (-29.04%) :arrow_down:
src/show.jl 57.44% <0%> (-26.43%) :arrow_down:
src/operations.jl 61.24% <0%> (-24.63%) :arrow_down:
src/slice.jl 69.31% <0%> (-21.73%) :arrow_down:
src/indexing/stringrange.jl 67.77% <0%> (-18.28%) :arrow_down:
... and 4 more

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 8ebef6b...eadd505. Read the comment docs.