Open KronosTheLate opened 2 years ago
Thanks for catching this and taking the time to write up a solution! That example is outdated and hasn't been touched in a while.
So the example is broken.
Flux.jl removed the included datasets a while ago, so this needs to be updated to use MLDatasets.jl.
I think the example would do great by using MLUtils instead of DataLoaders.jl and MLDataPattern. Also, Flux imports DataLoader so no need to explicitly import it.
I agree and am working on deprecating DataLoaders.jl since I've added all its functionality to MLUtils.jl. While FluxTraining.jl is agnostic of the kind of data iterator, I agree it would be good to standardize on MLUtils.jl.
However, when I try to train my learner, it seems like a single float is passed to predict, and not an array:
The setup you wrote is correct, but you then passed train_data
to fit!
instead of train_loader
. fit!
expects an iterator over batches so the latter would be correct. Passing the complete train_data
which is a single array, means that the training loop will iterate over the array, thus getting a single float at every iteration.
To improve the reliability of this package, could doc testing be used to ensure that in the future, the documentation examples actually run?
This is done for the majority of code in the docs, but not for this page. The reason is that the CI does not have a GPU available to train the model, which is why the source for this page is an executed Jupyter notebook. The problem with this is that it has to be manually rerun.
If you'd like to contribute your proposed changes as a PR, I'd be happy to review it!
It would boil down to:
DataLoader
s to fit!
, instead of the complete arraysI did not realize that you were the one behind MLUtils
xD Feeling a little dumb for telling you about it now.
However, your proposed fix still does not work for me. The line that defines lossfunc
turns red for me, indicating that it is the where things go wrong, even though I could not see it in the stacktrace:
Unlike Flux.jl, FluxTraining.jl separates the loss function and the model, so that model outputs can be stored. That means that the loss function going into the Learner
should be just the Flux loss function, i.e. Learner(predict, Flux.Losses.logitcrossentropy)
.
Then it works fine:
I made some minor adjustments to the data loading code as well, the full code is here:
using Flux
using Flux: onehotbatch, onecold
using FluxTraining
using MLUtils: flatten, unsqueeze
using MLDatasets
const LABELS = 0:9
function preprocess((data, targets))
return unsqueeze(data, 3), onehotbatch(targets, LABELS)
end
traindata = MNIST(Float32, :train)[:] |> preprocess
testdata = MNIST(Float32, :test)[:] |> preprocess
trainloader = DataLoader(traindata, batchsize=128);
testloader = DataLoader(testdata, batchsize=128);
model = Chain(flatten, Dense(28^2, 10));
lossfn = Flux.Losses.logitcrossentropy
optimizer = ADAM()
callbacks = [Metrics(accuracy)]
learner = Learner(predict, lossfn; optimizer, callbacks)
FluxTraining.fit!(learner, 1, (trainloader, testloader))
That makes sense. I always found it weird that I had to define the loss function in terms of the model. Also, big fan of the changes you made. It looks great.
I do have my hands full, but as some point I will find time to update the docs. But I really think that at the very least a functional MWE should be put in the docs before I will find the time to go down your checklist. If nothing else, a temporary warning and a link to this issue would do the trick.
Getting the following error from you code:
julia> using MLDatasets
julia> traindata = MNIST(Float32, :train)[:] |> preprocess
ERROR: MethodError: objects of type Module are not callable
Stacktrace:
[1] top-level scope
@ REPL[135]:1
I will just stick with traindata = MNIST.traindata(Float32) |> preprocess
I think your version of MLDatasets.jl is not up-to-date, so you may wanna update to 0.7.1, i.e. ]add MLDatasets@0.7.1
You are right! However, with the latest version the output is of type MNIST<:MLDatasets.SupervisedDataset
, not a tuple. The following works:
function preprocess(data::MNIST)
return unsqueeze(data.features, 3), onehotbatch(data.targets, LABELS)
end
# traindata and testdata contain both inputs (pixel values) and targets (correct labels)
traindata = MNIST(Float32, :train) |> preprocess;
testdata = MNIST(Float32, :test) |> preprocess;
The example Training an image classifier currently uses the following code:
However,
So the example is broken. As a side note, I think the example would do great by using MLUtils instead of
DataLoaders.jl
andMLDataPattern
. Also, Flux imports DataLoader so no need to explicitly import it.But I take a look at the docs and try to get started. So I make the following code, that works with Flux's base capacities:
At this point, I start checking loss and training with Flux's
train!
:Training no problem. However, when I try to train my learner, it seems like a single float is passed to
predict
, and not an array:I am completely stuck as to what goes wrong. Pointers in that regard would be appreciated, but the main issue is making the example functional, and updating the packages used to load data and the utility functions that I take from
MLUtils
.To improve the reliability of this package, could doc testing be used to ensure that in the future, the documentation examples actually run?