Various comments

[bkamins]

The index is stored in the Index column of coredata.

x-link to Index is missing

If coredata only has a single column then a new sequential index is generated.

1. What if it has no columns?
2. What if I want this single column already has name Index?

In general mention in the description the second argument in the constructor.

Mention that any Tables.jl table is also accepted.

TSFrames.jl/src/apply.jl

Line 149 in c2078d0

:Index => index_at => :Index,

What if index column is not called :Index? (the same with other places - the docs above said that the index column does not have to be called Index, but maybe I misunderstood)

TSFrames.jl/src/diff.jl

Line 79 in c2078d0

error("periods must be a postive int")

I would throw ArgumentError for easier error handling (also in other places)

TSFrames.jl/src/endpoints.jl

Line 321 in c2078d0

V<:Union{

Why not just Period (this would allow for supporting user extensions of special date/time types)

j::AbstractVector{T}) where {T<:Union{String, Symbol}}

This is minor, but it is incorrect as it allows:

julia> Union{String, Symbol}[:s, "s"]
2-element Vector{Union{String, Symbol}}:
 :s
 "s"

Indexing with string

1. You allow only String, while it would be better to allow for AbstractString
2. You do not allow for indexing with vector of Bool (neither rows nor columns) - maybe this is intended
3. You do not allow special selectors like Between, Not etc. (maybe this is intended?)

return (f == :coredata) ? getfield(ts, :coredata) : getproperty(ts.coredata, f)

This is incorrect. coredata can have :coredata column name. fields and properties should not be mixed. I would use a function to expose :coredata field instead. Eg. getdataframe(tsf::TSFrame) = getfield(tsf, :coredata)

Base.join

People did not like extending this function in packages. Better add methods for innerjoin etc.
You might consider allowing passing kwargs like renamecols.

DataFrames.Matrix(ts.coredata[!, Not(:Index)])

DataFrames prefix is not needed

general question (maybe I missed something)

Maybe you want TSFrame to be iterable? I.e. have length and support something like:

for row in tsf
# here have DataFrameRow with data
end

general question (maybe I missed something)

How do you want to support panel data (so essentially TSFrame grouped by some column)

[chiraganand]

Thanks @bkamins for the very useful comments. I have responded below.

The index is stored in the Index column of coredata.

x-link to Index is missing

Sorry, did not understand this. What is the purpose of cross linking?

If coredata only has a single column then a new sequential index is generated.

1. What if it has no columns?

There is a constructor for creating an empty TSFrame object but currently it does not work with a DataFrame without any columns. Ideally, TSFrame(DataFrame()) should create a TSFrame with an Index column and 0 other non-index columns and 0 rows.

2. What if I want this single column already has name `Index`?

Right now, TSFrame(DataFrame(Index=[1, 2])) throws an error because a DataFrame cannot be constructed with two Index column names. This only happens when the DataFrame has only one column named Index. In other cases, the column named Index is taken as the index column of TSFrame, this is the intended behaviour. The single column DataFrame implementation will have to be fixed.

In general mention in the description the second argument in the constructor.
Mention that any Tables.jl table is also accepted.

Yes, to both.

TSFrames.jl/src/apply.jl

Line 149 in c2078d0

:Index => index_at => :Index,

What if index column is not called :Index? (the same with other places - the docs above said that the index column does not have to be called Index, but maybe I misunderstood)

It means that the original DataFrame does not need to have a column named Index because the TSFrame index can be constructed using an existing column by specifying the column index (Int) or the name (Symbol/String).

If the column number or name isn't specified in the constructor then Index named column is looked up and if that doesn't exist then Index is generated.

TSFrames.jl/src/diff.jl

Line 79 in c2078d0

error("periods must be a postive int")

I would throw ArgumentError for easier error handling (also in other places)

Yes.

TSFrames.jl/src/endpoints.jl

Line 321 in c2078d0

V<:Union{

Why not just Period (this would allow for supporting user extensions of special date/time types)

Yes, of course. Got missed.

j::AbstractVector{T}) where {T<:Union{String, Symbol}}

This is minor, but it is incorrect as it allows:

julia> Union{String, Symbol}[:s, "s"]
2-element Vector{Union{String, Symbol}}:
 :s
 "s"

Thanks, did not know about this.

Indexing with string

1. You allow only `String`, while it would be better to allow for `AbstractString`

Yes.

2. You do not allow for indexing with vector of `Bool` (neither rows nor columns) - maybe this is intended
3. You do not allow special selectors like `Between`, `Not` etc. (maybe this is intended?)

Not that both of these are not intended but were lower priority earlier. I believe these would be good enhancements.

return (f == :coredata) ? getfield(ts, :coredata) : getproperty(ts.coredata, f)

This is incorrect. coredata can have :coredata column name. fields and properties should not be mixed. I would use a function to expose :coredata field instead. Eg. getdataframe(tsf::TSFrame) = getfield(tsf, :coredata)

Alright, will fix this.

Base.join

People did not like extending this function in packages. Better add methods for innerjoin etc. You might consider allowing passing kwargs like renamecols.

This is done to keep the API simple with a single join method supporting all kinds of joins. R zoo/xts follow this convention.

DataFrames.Matrix(ts.coredata[!, Not(:Index)])

DataFrames prefix is not needed

Okay.

general question (maybe I missed something)

Maybe you want TSFrame to be iterable? I.e. have length and support something like:

for row in tsf # here have DataFrameRow with data end

This is possible with Tables.jl implementation: for row in Tables.rows(tsf)

general question (maybe I missed something)

How do you want to support panel data (so essentially TSFrame grouped by some column)

There hasn't been much thought about taking this path actually. Not sure if TSFrames should handle panel data use cases or another package which uses composition with TSFrame?

[bkamins]

What is the purpose of cross linking?

So that in the generated documentation you can click on Index and go to its definition.

[chiraganand]

The list of issues to track some of your comments:

1. Documentation: add cross-references #182
2. Constructor: Create empty TSFrame if an empty DataFrame is provided #183
3. Empty DataFrame constructor should not output a TSFrame with 1 column  #184
4. DataFrame only containing one column named Index breaks the constructor  #185
5. Add consistency checks to functions where :Index column name is directly used #186
6. Throw correct error types in functions #187
7. endpoints.jl: Use <: Dates.Period instead of listing all the sub-types #188
8. Don't use Union{String, Symbol} in getindex #189
9. Accept AbstractString instead of String in function arguments #190
10. getindex(): allow for Bool indexing for both rows and columns #191
11. getindex(): implement indexing with special operators: Between(), And(), Not(), etc. #192
12. Disallow Vector{Union{String, Symbol}} to have both types in a single vector #194