Update examples/MNIST Manifest, including Julia 1.10 #254

ablaom · 2024-05-24T04:39:59Z

This PR resolves #234. It updates the example/MNIST to use latest package versions and Julia 1.10

To do:

Test notebook locally with GPU switched on (acceleration=CUDALibs()).

cc @EssamWissam

codecov · 2024-05-26T22:03:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.11%. Comparing base (b449d80) to head (638a19c).
Report is 48 commits behind head on dev.

❗ Current head 638a19c differs from pull request most recent head 0afbff1

Please upload reports for the commit 0afbff1 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #254      +/-   ##
==========================================
+ Coverage   92.08%   92.11%   +0.02%     
==========================================
  Files          12       12              
  Lines         316      317       +1     
==========================================
+ Hits          291      292       +1     
  Misses         25       25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EssamWisam · 2024-05-30T09:51:38Z

examples/mnist/notebook.jl

@@ -77,7 +77,7 @@ function MLJFlux.build(b::MyConvBuilder, rng, n_in, n_out, n_channels)
        MaxPool((2, 2)),
        Conv((k, k), c2 => c3, pad=(p, p), relu, init=init),


Instead of computing the required padding to perserve image size manually above and passing it here, one can just pass the singleton SamePad()
https://fluxml.ai/Flux.jl/stable/models/layers/#Convolution-Models

Good to know. I will leave it as is, for less "hidden knowledge" for beginners, but will add your alternative in the form of a comment.

EssamWisam · 2024-05-30T09:53:47Z

examples/mnist/notebook.jl

+    batch_size=50,
+    epochs=10,
+    rng=123,
+)

 # You can add Flux options `optimiser=...` and `loss=...` here. At


Suggested change

# You can add Flux options `optimiser=...` and `loss=...` here. At

# You can add to the constructor Flux options `optimiser=...` and `loss=...` here. At

because its later mention To run on a GPU, set acceleration=CUDALib() and omit rng. which if followed by a call to machine (so one may think they should add it the machine and not the constructor if that's not explicit.)

Addressed in a new commit.

EssamWisam · 2024-05-30T09:57:02Z

examples/mnist/notebook.jl

@@ -129,7 +130,7 @@ fit!(mach, rows=1:500);
 # Computing an out-of-sample estimate of the loss:

 predicted_labels = predict(mach, rows=501:1000);
-cross_entropy(predicted_labels, labels[501:1000]) |> mean
+cross_entropy(predicted_labels, labels[501:1000])


I do personally prefer training and testing data to be separate data structures (or at least to make this more generic by deciding a split size initially that could be changed as needed).

It may confuse some that we are loading the training data of MNIST then using the first 500 examples for actual training and the second 500 for testing/validation.

examples/mnist/notebook.jl

EssamWisam · 2024-05-30T10:01:30Z

examples/mnist/notebook.jl

@@ -63,8 +65,6 @@ struct MyConvBuilder
    channels3::Int
 end

-make2d(x::AbstractArray) = reshape(x, :, size(x)[end])


There is another instance of this below around line 173, should it be also replaced with MLUtils.flatten?

Okay, it looks like make2d is not used, and we can use vec for make1d. So I've done that.

EssamWisam · 2024-05-30T10:03:43Z

examples/mnist/notebook.jl


-# **Note.** The the higher the number, the deeper the chain parameter.
+# **Note.** The higher the number, the deeper the chain parameter.


I don't get in what sense its deeper because perhaps this is using the same number of hidden layers regardless to epochs.

We are reporting the mean value of weights for different layers. The higher the index, the deeper the layer we are averaging.

Added a comment to clarify, but let me know if this is still not clear or if you believe my assertion is not correct.

OK. I hadn't seen the plot or noticed the div operation when I commented. Now I understand that the higher the index in the means list, the deeper the layer. I think I initially understood "number" to be the epoch number.

Those we can either just assume users will conclude that "number" is the index in the plot or clarify it.

@ablaom this comment.

Co-authored-by: Essam <[email protected]>

…nto mnist-update

ablaom · 2024-06-06T04:55:22Z

OKay @EssamWisam I'm ready for you to have another look at this.

EssamWisam · 2024-06-10T18:10:49Z

OKay @EssamWisam I'm ready for you to have another look at this.

Redefines perfection 👌. I think we are ready to merge this just after we resolve anything, if needed, in my comment above.

EssamWisam · 2024-06-10T18:14:04Z

Oh and also after exposing it in the documentation: obvious by moving the tutorial to the docs folder and then putting the path in make.jl (in the commented tutorials section).

ablaom · 2024-06-10T23:12:38Z

I think we are ready to merge this just after we resolve anything, if needed, in my comment above.

Sorry, which comment is that?

Oh and also after exposing it in the documentation: obvious by moving the tutorial to the docs folder and then putting the path in make.jl (in the commented tutorials section).

Mmm. I'm going to postpone that step, but thanks for flagging.

update examples/MNIST Manifest, including Julia 1.10

1dfad77

ablaom marked this pull request as draft May 24, 2024 04:40

add some clarificatioins about GPU case in MNIST example

638a19c

fix a Literate issue with code comments

3f16411

ablaom marked this pull request as ready for review May 26, 2024 22:19

ablaom requested a review from EssamWisam May 26, 2024 22:21

EssamWisam reviewed May 30, 2024

View reviewed changes

examples/mnist/notebook.jl Outdated Show resolved Hide resolved

EssamWisam reviewed May 30, 2024

View reviewed changes

ablaom and others added 5 commits June 6, 2024 14:30

introduce variables for the test and train indices

4fcc61a

commit review suggestion

9f76694

Co-authored-by: Essam <[email protected]>

address remaining review comments. New notebook.ipynb not executed!

d3e4662

Merge branch 'mnist-update' of https://github.com/FluxML/MLJFlux.jl i…

ed4b9bd

…nto mnist-update

generate a new notebook.ipynb (executed) for examples/mnist

0afbff1

ablaom merged commit 088e152 into dev Jun 10, 2024
6 checks passed

ablaom deleted the mnist-update branch June 10, 2024 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update examples/MNIST Manifest, including Julia 1.10 #254

Update examples/MNIST Manifest, including Julia 1.10 #254

ablaom commented May 24, 2024 •

edited by EssamWisam

Loading

codecov bot commented May 26, 2024 •

edited

Loading

EssamWisam May 30, 2024

ablaom Jun 6, 2024 •

edited

Loading

EssamWisam May 30, 2024

ablaom Jun 6, 2024

EssamWisam May 30, 2024

ablaom Jun 6, 2024

EssamWisam May 30, 2024

ablaom Jun 6, 2024

EssamWisam May 30, 2024

ablaom Jun 6, 2024

EssamWisam Jun 10, 2024

EssamWisam Jun 10, 2024

ablaom commented Jun 6, 2024

EssamWisam commented Jun 10, 2024

EssamWisam commented Jun 10, 2024

ablaom commented Jun 10, 2024

		@@ -77,7 +77,7 @@ function MLJFlux.build(b::MyConvBuilder, rng, n_in, n_out, n_channels)
		MaxPool((2, 2)),
		Conv((k, k), c2 => c3, pad=(p, p), relu, init=init),

	# You can add Flux options `optimiser=...` and `loss=...` here. At
	# You can add to the constructor Flux options `optimiser=...` and `loss=...` here. At


		# Note. The the higher the number, the deeper the chain parameter.
		# Note. The higher the number, the deeper the chain parameter.

Update examples/MNIST Manifest, including Julia 1.10 #254

Update examples/MNIST Manifest, including Julia 1.10 #254

Conversation

ablaom commented May 24, 2024 • edited by EssamWisam Loading

codecov bot commented May 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ablaom Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ablaom commented Jun 6, 2024

EssamWisam commented Jun 10, 2024

EssamWisam commented Jun 10, 2024

ablaom commented Jun 10, 2024

ablaom commented May 24, 2024 •

edited by EssamWisam

Loading

codecov bot commented May 26, 2024 •

edited

Loading

ablaom Jun 6, 2024 •

edited

Loading