Skip to content
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

Simpler MLP #397

Merged
merged 8 commits into from
May 22, 2023
Merged

Simpler MLP #397

merged 8 commits into from
May 22, 2023

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Apr 25, 2023

Same idea as #394, to remove all this baroque junk of making structs you use once, and functions you call once, to just focus on the essentials. But even simpler than #394, no GPU, no NamedTuples, no saving.

Maybe some of the text in the old one should move to the readme.

[Edit: fixed links to work after merge.]

@mcabbott mcabbott added the update making a model work with latest Flux, etc label Apr 25, 2023
Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One benefit of all the extra comments in the original was that it could be rendered as a Literate notebook. I wonder if we should make that official with either a README note or top-level comment with brief instructions.

vision/mlp_mnist/mlp_mnist.jl Outdated Show resolved Hide resolved
vision/mlp_mnist/mlp_mnist.jl Show resolved Hide resolved
vision/mlp_mnist/README.md Outdated Show resolved Hide resolved

## Training

You can copy and paste the example into the Julia REPL to see what each part does.
Or you can run it all at once from the terminal, like this:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running all at once from the terminal won't show any output. Could we use print on the predictions and display the image even if users aren't running from the REPL?

Copy link
Member Author

@mcabbott mcabbott Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it does print:

mlp_mnist % /Applications/Julia-1.9.app/Contents/Resources/julia/bin/julia --project mlp_mnist.jl
┌ Info: After epoch = 1
│   loss = 1.8101638024672866
│   train_acc = 75.81
└   test_acc = 76.75
┌ Info: After epoch = 3
│   loss = 0.8674804433248937
│   train_acc = 85.65
└   test_acc = 86.22

It does not show the images, but perhaps that's more intended as a start towards interactive exploration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not the images, perhaps we could print out the predictions/ground truth labels in lines 110+115 for comparison.

Co-authored-by: Brian Chen <ToucheSir@users.noreply.github.com>
@mcabbott
Copy link
Member Author

mcabbott commented Apr 26, 2023

original was that it could be rendered as a Literate notebook

I think quite a few of these files were rendered this way for the old website. But they got stale... and IDK how valuable rendering & commentary is anyway.

This PR and its friend want to go the other direction. Having relatively few, concise, clear examples means you're more likely to find the right one, and it's more likely to be up to date. A separate README for links & more text seems OK, and easier to maintain, less tightly coupled.

vision/mlp_mnist/README.md Outdated Show resolved Hide resolved
vision/mlp_mnist/mlp_mnist.jl Outdated Show resolved Hide resolved
vision/mlp_mnist/mlp_mnist.jl Outdated Show resolved Hide resolved
vision/mlp_mnist/mlp_mnist.jl Outdated Show resolved Hide resolved
Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>
@ToucheSir ToucheSir mentioned this pull request May 21, 2023
@mcabbott mcabbott merged commit 31e147f into master May 22, 2023
@mcabbott mcabbott deleted the simpler_mlp branch May 22, 2023 23:50
@mcabbott mcabbott mentioned this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
update making a model work with latest Flux, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants