New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Time Series Block #239
Conversation
Hey for downloading the datasets, the message as well as the post fetch method needs to be different which I think might require some restructuring, how should I go about that ? |
src/datasets/containers.jl
Outdated
elseif startswith(ln, "@timestamps") | ||
# Check that the associated value is valid | ||
tokens = split(ln, " ") | ||
token_len = length(tokens) | ||
|
||
if tokens[2] == "true" | ||
timestamps = true | ||
else | ||
timestamps = false | ||
end | ||
|
||
has_timestamps_tag = true | ||
metadata_started = true | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a way to extract out some of this logic. For now, having tests would help verify it's working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it could be extracted out for the current datasets we are using. I just copied it over from python library to get it working.
By tests do you mean automated tests and creating a sample .ts
file to run the test on ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. You could also use a small existing one as long as the license is compatible.
|
||
""" | ||
|
||
struct TimeSeriesRow{M,N} <: Block end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of the block name here. I would prefer TimeSeries
, but that's already the module name. Once #240 is through, the time-series functionality will be a subpackage FastTimeSeries
, so then the name TimeSeries
for the block would be available. Let's leave it for now, and maybe change it then.
|
||
""" | ||
|
||
struct TimeSeriesRow{M,N} <: Block end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both the number of features and the observation length as type parameters? We should only do this if we need to dispatch on the number of features or observation length.
Additionally, a Block
is constant for a dataset, so including the observation length means we wouldn't be able to support datasets where different samples have varying observation lengths. Is that the case for any of the datasets we're using? Do we need this information somewhere on the block-level? If we don't need it, I would suggest dropping the observation length from the block or allowing passing a colon :
to allow variable-length observations.
Also, if we don't need to dispatch on the number of features (do we?), it can be added as a field.
So we'd have something like
struct TimeSeriesRow <: Block
nfeatures::Int
obslength::Union{Int, Colon}
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For the current datasets we are planning to use, the length is same for all observations, so we can have a constant block.
From my understanding we build the model from the block ? So the parameters might depend on time series length and number of variables.
So ideally the block should just dispatch on parameters which are required in model building ?
src/TimeSeries/recipes.jl
Outdated
# ## Tests | ||
|
||
@testset "TimeSeriesDataset [recipe]" begin | ||
path = datasetpath("ecg5000") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how big this dataset is, but if it's really big, we may not want to run this on the CI, since it'll need to download it every time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dataset is around 10mb, should we run this on CI ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be fine 👍
src/datasets/containers.jl
Outdated
table::AbstractArray{Float64,3} | ||
end | ||
|
||
function LearnBase.getobs(dataset::TimeSeriesDataset, idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated to Base.getindex
and Base.length
now that #229 is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will update it.
rebased with master
89a0de1
to
dd57498
Compare
I have implemented the changes we discussed in the last call.
|
Great. Just to be safe, I think it would be best to download the file on the fly for now. |
…t) (#244) Co-authored-by: CompatHelper Julia <compathelper_noreply@julialang.org>
function setup(::Type{TSPreprocessing}, ::TimeSeriesRow, data) | ||
means, stds = tsdatasetstats(data) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function setup(::Type{TSPreprocessing}, ::TimeSeriesRow, data) | |
means, stds = tsdatasetstats(data) | |
end | |
setup(::Type{TSPreprocessing}, ::TimeSeriesRow, data) = means, stds = tsdatasetstats(data) |
src/TimeSeries/TimeSeries.jl
Outdated
export | ||
TimeSeriesRow, TSClassificationSingle, TimeSeriesPreprocessing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export | |
TimeSeriesRow, TSClassificationSingle, TimeSeriesPreprocessing | |
export | |
TimeSeriesRow, TSClassificationSingle, TSPreprocessing |
function TSPreprocessing() | ||
base_tfms = [ | ||
] | ||
return TSPreprocessing(base_tfms) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of transforms will be in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently only Standardize, that's the only used in the tutorials.
If time permits we can also add normalisation using min-max, clipping outliers based on IQR, handle missing values in the time series.
function encodedblock(p::TSPreprocessing, block::TimeSeriesRow) | ||
return block | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the format of the time series is changed by the encoding, this should return a different block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the format won't be changed, as I discussed with Brian earlier that different models might require different formats and so the encoding shouldn't depend on the model.
I have added the basic structure and code. There seems to be a couple of errors, I will solve by tonight. |
* Move domain-specific functionality to subpackages * Add FastMakie.jl * Update tests * Add subpackage CI * run SciMLStyle * Add sysimage example * Update documentation * Rerun notebooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments, but let me know when you're done with the other changes :) 👍
struct TSStats{T} | ||
means::AbstractArray{T,2} | ||
stds::AbstractArray{T,2} | ||
end | ||
|
||
function TSStats(means, stds) | ||
TSStats{eltype(means)}(means, stds) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need this struct, it may be simpler to add means
and stds
fields to the Encoding
. Then that also makes it easier to construct TSPreprocessing
manually (i.e. without setup
).
obs = tfm(obs) | ||
end | ||
obs | ||
function TSStandardize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an encode
method?
end | ||
axes = [ax for ax in [1, 2, 3] if !(ax in drop_axes)] | ||
mean = Statistics.mean(data.table, dims=axes) | ||
std = Statistics.std(data.table, dims=axes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std = Statistics.std(data.table, dims=axes) | |
std = Statistics.std(data.table, dims=axes, mean=mean) |
If the data source supports it, this is more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried using Statistics.stdm(itr, mean)
earlier, couldn't seem to get it working. Will look into it further and keep this comment open till then.
means = reshape(means, ( size( means)[2:3] )) | ||
stds = reshape(stds, ( size( stds)[2:3] )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little weird formatting going on here. Why is the reshape needed?
src/datasets/containers.jl
Outdated
if class_labels | ||
return data, class_val_list | ||
else | ||
return data | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning one or two things based on a conditional is a little surprising. Consider either always returning class_val_list and/or making the second return value some meaningful null value (nothing, empty array, whatever makes the most sense).
means::AbstractArray{T,2} | ||
stds::AbstractArray{T,2} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also consider changing these to AbstractMatrix{T}
, but it would be best to confirm first that they will always be 2-dimensional (what about higher-dimensional time series, for example?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the higher dimensional series, I will have to check the literature or some examples online if I can find them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I can come up with examples easily enough, the question is whether the fastai docs have any :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I will check that up before our meeting today.
Hey the encodings are working now. > using FastAI
> data, blocks = load(datarecipes()["ecg5000"]);
> task = FastAI.TSClassificationSingle(blocks, data);
> input, target = getobs(data, 2)
(Float32[-1.1008778 -3.9968398 … 1.1196209 -1.4362499], "1")
> encodesample(task, Training(), (input, target))
(Float32[-1.1048299 -4.011188 … 1.1236403 -1.4414059], Float32[1.0, 0.0, 0.0, 0.0, 0.0]) I am looking to add some tests as well look to resolve all the comments you guys made before the meeting. Have also started working on creating the demo notebook side-by-side as you guys suggested. |
Could you push the notebook here as well? You can just place it in the |
Since I merged #240, this shows a lot of merge conflicts, but don't worry, I will do the merge myself manually once this PR is ready 👍 |
Rendering of Unicode plots can be a bit off depending on the notebook environment/viewer. It should end up looking fine in the docs, though. For regular usage, the Makie backend will probably preferable, but we can get to that later 👍 |
Having some trouble to push the subpackage merge to your fork, @codeboy5. Can you check that I have permissions to push to your fork? Or, alternatively just give me committer access to the fork |
If checked, users with write access to FluxML/FastAI.jl can add new commits to your timeseries-blocks branch. You can always change this setting later. |
Yeah, please try that |
Yeah, just sent you an invite for the same. Hopefully it should work now. |
I merged this PR into master, so it now lives in the subpackage FastTimeSeries. The CI is failing right now for unrelated reasons, I'll rerun it when those are fixed (by #247) |
With this commit, we are able to the following now > using FastAI, FastTimeSeries
> data, blocks = load(datarecipes()["ecg5000"]);
> task = FastTimeSeries.TSClassificationSingle(blocks, data);
> backbone = FastTimeSeries.Models.StackedLSTM(1, 16, 10, 2);
> model = FastAI.taskmodel(task, backbone)
Chain(
StackedLSTMCell(
Recur(
LSTMCell(1 => 10), # 500 parameters
),
Recur(
LSTMCell(10 => 16), # 1_760 parameters
),
),
identity,
Dense(16 => 5), # 85 parameters
) # Total: 12 trainable arrays, 2_345 parameters,
# plus 4 non-trainable, 13_312 parameters, summarysize 62.145 KiB. |
function StackedLSTM(in::Int, out::Integer, hiddensize::Integer, layers::Integer; | ||
init=Flux.glorot_uniform) | ||
if layers == 1 | ||
chain = Chain(LSTM(in, out; init=init)) | ||
elseif layers == 2 | ||
chain = Chain(LSTM(in, hiddensize; init=init), | ||
LSTM(hiddensize, out; init=init)) | ||
else | ||
chain_vec = [LSTM(in, hiddensize; init=init)] | ||
for i = 1:layers - 2 | ||
push!(chain_vec, LSTM(hiddensize, hiddensize; init=init)) | ||
end | ||
chain = Chain(chain_vec..., LSTM(hiddensize, out; init=init)) | ||
end | ||
return StackedLSTMCell(chain) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is a little weird here: you're returning a Cell
, but inside that cell are Recur
s and not LSTMCell
s. Ideally StackedLSTMCell
would be structured like the other Flux RNN cells, but if that's not possible I'd recommend renaming to StackedLSTM
to better represent the model as stateful + containing internal mutation (transitively via Recur
from LSTM
).
function initialize_bias!(l::StackedLSTMCell) | ||
for i = 1:length(l.chain) | ||
l.chain[i].cell.b .= 1 | ||
end | ||
return nothing | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done explicitly through the initb
argument of the LSTM(Cell) constructor if possible.
[m.chain(x) for x ∈ X[1:end-1]] | ||
return m.chain(X[end]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this always a sequence-to-one model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for classification and regression, this will always be a seq-to-one model ?
For some nlp tasks, it would not be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I think you should be able to rewrite this using foldl
. It'll be more correct and likely perform better as well. It may also let you drop the Recur
wrapper for the inner layer stack and use the cells directly instead (i.e. LSTM -> LSTMCell
).
FastTimeSeries/src/models/RNN.jl
Outdated
@@ -24,5 +30,5 @@ function RNNModel(recbackbone, | |||
dropout_rate = 0.0) | |||
|
|||
dropout = dropout_rate == 0 ? identity : Dropout(dropout_rate) | |||
Chain(recbackbone, dropout, finalclassifier) | |||
end | |||
Chain(tabular2rnn, recbackbone, dropout, finalclassifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be doing this dense to slices transform in the model itself. If you just need something RNN friendly, relying on the built-in support for dense inputs should be enough. The permutedims
can stay for now, but even that probably shouldn't be in the gradient hot path (it allocates O(n) both ways).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative for now is to make the data pipeline spit out the vector of arrays. We can then revisit if/when you add models like CNNs which expect dense inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "ideal" place to do this transform would be inside the training loop. I am not sure how to exactly do that for FastAI.jl. Is there a way ?
Since the second phase would involve using some CNNs, using data pipeline to spit out vector of arrays would not work.
FastTimeSeries/src/models/RNN.jl
Outdated
@@ -1,3 +1,9 @@ | |||
function tabular2rnn(X::AbstractArray{Float32, 3}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "tabular" is a bit of a misnomer here, but naming is not a priority.
Since this PR is getting quite long, it may be best to move the newest changes to a new PR and merge the parts that are ready. We could revert the PR to the point where I did the manual merge, merge this PR with the changes up to that point into master, and then open a new PR based on master with the more recent changes, including models and so on. @codeboy5 let me know if you aren’t sure about how to do this 🙂 |
Yeah sure, just have to revert the code to the pr right ? |
Yeah this looks good! Just to make sure, @darsnack does it make sense to rebase this on master when merging or should we squash and merge? |
Either is okay. Maybe squash and merge will be less painful? |
I've squashed and merged it. @codeboy5 you should be able to open a new PR for the model code based on master now. |
will do thanks . 👍🏻 |
Added Time Series Container and Block. It is capable of loading all datasets from timeseriesclassification. The
.ts
files are loaded using Julia translation of this method .I have also added a basic test case for the recipe.
This allows us to do the following
Just wanted to get some initial thoughts on the work, there might be more changes as I continue to work on the other parts.