-
Notifications
You must be signed in to change notification settings - Fork 3
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
Move KnetNLPModel example from JOSS #69
Conversation
paraynaud
commented
Jun 30, 2023
- fix source code, remove Chain effectively fix Remove KnetNLPModels.Chain #64
e0a374e
to
304b866
Compare
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.
Really good overall,
I just added small suggestions,
Let me know
docs/src/LeNetTraining.md
Outdated
@@ -0,0 +1,123 @@ | |||
# Training a LeNet architecture with JSO optimizers |
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.
Can we use LeNet_training.md as a name, so we are consistence with JSO style
docs/src/LeNetTraining.md
Outdated
``` | ||
|
||
## MNIST dataset loading | ||
Accordingly to LeNet architecture, we chose the MNIST dataset [@lecun-bouttou-bengio-haffner1998] from MLDataset: |
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.
Does it find the reference or we need to add references manually ?
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.
It will not find it.
Thanks for the catch, I will fix this soon.
|
||
dtrn = minibatch(xtrn, ytrn, 100; xsize=(size(xtrn, 1), size(xtrn, 2), 1, :)) # training minibatch | ||
dtst = minibatch(xtst, ytst, 100; xsize=(size(xtst, 1), size(xtst, 2), 1, :)) # test minibatch | ||
dtrn = minibatch(xtrain, ytrain, 100; xsize=(size(xtrain, 1), size(xtrain, 2), 1, :)) # training minibatch |
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.
would it be better to create a variable call b_size and pass it so the reader understand the batch size is 100
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.
We should clarify (homogenize) the minibatch sizes.
Because, in this case, 100 indicates that each minibtach is a hundreth (1/100) of the given dataset.
For example, a MNIST training minibatch is of size 600 (60000/100)
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 am good with this, @dpo should I approve it (first review :) )
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 am good with this, @dpo should I approve it (first review :) )