TimeSeries.jl icon indicating copy to clipboard operation
TimeSeries.jl copied to clipboard

Why timestamp instead of timestamps?

Open JeffreySarnoff opened this issue 8 years ago • 9 comments

It seems that timestamp(ts) and value(ts) or timestamps(ts) and values(ts) would be more consistent. (a minor issue)

JeffreySarnoff avatar Jun 07 '17 21:06 JeffreySarnoff

eh

JeffreySarnoff avatar Jun 08 '17 21:06 JeffreySarnoff

Yeah, not sure how much of others' code it would break. Of course we could go the deprecation route to mitigate that issue.

milktrader avatar Jun 09 '17 11:06 milktrader

@milktrader thanks for the note .. let's deprecate it -- the advantage is to new users, and naturalness in soft-terms is its own reward.

also ..

JeffreySarnoff avatar Jun 09 '17 12:06 JeffreySarnoff

We have a deprecated.jl file in the /src directory.

Both plural or both singular?

milktrader avatar Jun 09 '17 12:06 milktrader

It's more of a stretch to say values are a value than it it to say a timestamp is timestamps so I would vote for going plural.

timestamp -> timestamps

We already have values

milktrader avatar Jun 09 '17 12:06 milktrader

Also fwiw, the original names come from the name of the type parameters.

ts.values ts.timestamp

milktrader avatar Jun 09 '17 12:06 milktrader

both plural because it is a rare event to have a TimeArray with a single timestamp and a values array that is a vector with one entry

The underlying reason to use the plural forms here parallels our convention to name a module that exports a type in the plural so the exported type can be named in the singular (IndexedTables, IndexedTable). TimeArrays hold two fields that provide any of many valuations specific to the fields. they are plural.

JeffreySarnoff avatar Jun 09 '17 12:06 JeffreySarnoff

After thinking about this for a few days I'm not sure we need to change the timestamp method name to timestamps. Here is my reasoning.

timestamp is a field of the TimeArray type. The timestamp method is nothing more than a wrapper for that field.

###### element wrapers ###########

timestamp{T,N}(ta::TimeArray{T,N}) = ta.timestamp
values{T,N}(ta::TimeArray{T,N})    = ta.values
colnames{T,N}(ta::TimeArray{T,N})  = ta.colnames
meta{T,N}(ta::TimeArray{T,N})      = ta.meta

The change of timestamp to timestamps would stand out as incongruous and odd. I do get the point that it's a stretch to think of timestamp as a series of timestamps. If anything, I would be in favor of removing this code altogether.

milktrader avatar Jun 12 '17 11:06 milktrader

There has been an effort to encourage exactly that use of fieldnames as functional accessors to the verysame field -- and to discourage immediate use of the .fieldname form outside of very low level ops. AFAIK that is still the preference -- but I have not discussed it recently. Some of the desire is to protect the specifics of a type implementation from becoming necessary furnature.

On Mon, Jun 12, 2017 at 7:20 AM, milktrader [email protected] wrote:

After thinking about this for a few days I'm not sure we need to change the timestamp method name to timestamps. Here is my reasoning.

timestamp is a field of the TimeArray type. The timestamp method is nothing more than a wrapper for that field.

element wrapers

timestamp{T,N}(ta::TimeArray{T,N}) = ta.timestampvalues{T,N}(ta::TimeArray{T,N}) = ta.valuescolnames{T,N}(ta::TimeArray{T,N}) = ta.colnamesmeta{T,N}(ta::TimeArray{T,N}) = ta.meta

If anything, I would be in favor of removing this code altogether.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/JuliaStats/TimeSeries.jl/issues/315#issuecomment-307761585, or mute the thread https://github.com/notifications/unsubscribe-auth/ABmqxgmygz7gPQNQa_FTvVom9DumZq6Sks5sDR7igaJpZM4NzTHM .

JeffreySarnoff avatar Jun 12 '17 11:06 JeffreySarnoff