Created a minibatch iterator selection function #57

farhadrclass · 2022-10-14T21:13:57Z

as discussed here #52

paraynaud

Thanks, and don't forget to add some tests!

src/utils.jl

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

farhadrclass · 2022-10-15T19:55:36Z

I will re-factor the code and also add the tests now

paraynaud

Thanks @farhadrclass, i suggested some minor changes!

src/utils.jl

src/KnetNLPModels.jl

Project.toml

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

farhadrclass · 2022-10-21T15:02:56Z

@paraynaud for some reason the doc builder fails
since it needs knetnlpmodels to run the example and we haven't push the PR, it doesn't have the functions we just wrote

we have to option
accept the PR and it should fix this or make a new one and remove the tutorial to that one

docs/src/tutorial.md

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

paraynaud

I checked the documentation of the new methods.
I made some changes to ease the understanding.
If you agree with the new doc, we will ask Dominique to finally review the PR.

src/utils.jl

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

paraynaud · 2022-10-25T15:15:09Z

@dpo, this one should be ready to go.

docs/src/tutorial.md

dpo · 2022-10-28T01:32:20Z

src/KnetNLPModels.jl

@@ -99,6 +103,8 @@ function KnetNLPModel(
    x0,
    layers_g,
    nested_array,
+    1, #inti the batch current i to 1


I don't understand the comment. Index of the first minibatch? Normally, you don't index iterators. You just loop through them.

yes but this is because Knet doesn't use the iterator correctly

The way knet does it is to ignore the size and do {i \in [1,data_size-batch_size]} which is terrible but that mean i for batch size of 1000 in MNIST is between 1 to 59999 which is weird for me but that is how it was written (I spent 2 days debugging till I find this weird behavior )

check here the knet random code: https://github.com/denizyuret/Knet.jl/blob/e9bec84b826ee7231db4ca8772fae6eb2fd4bcb9/src/train20/data.jl

function rand(d::Data)
i = rand(0:(d.length-d.batchsize))
return iterate(d, i)[1]
end

So basically their iterator is using i and i actually is the index of the data not the mini-batch,
I am open to suggestion how to fix this since I think the problem is Knet's iterator is to some extend weird

Should "inti" be "init" ? Better write "initialize".

src/utils.jl

dpo · 2022-10-28T01:33:25Z

src/utils.jl

-  nlp.current_training_minibatch = rand(nlp.training_minibatch_iterator)
+function reset_minibatch_train!(nlp::AbstractKnetNLPModel)
+  nlp.current_training_minibatch = first(nlp.training_minibatch_iterator)
+  nlp.i_train = 1


Here, we see that the index 1 is not useful.

we use the index 1 as a flag for when data is visited or new iteration has started

src/utils.jl

Co-authored-by: Dominique <dominique.orban@gmail.com>

…odels.jl into PR_utilies

no need for return 1 since our reset_minibatch_.. does reset them

dpo

Thanks! Just a few minor comments. You can merge after that.

docs/src/tutorial.md

dpo · 2022-11-04T12:52:34Z

src/KnetNLPModels.jl

@@ -99,6 +103,8 @@ function KnetNLPModel(
    x0,
    layers_g,
    nested_array,
+    1, #inti the batch current i to 1


Should "inti" be "init" ? Better write "initialize".

src/KnetNLPModels.jl

Co-authored-by: Dominique <dominique.orban@gmail.com>

Created a minibatch iterator selection function

eacf4b0

farhadrclass mentioned this pull request Oct 14, 2022

JOSS article #52

Closed

paraynaud requested changes Oct 14, 2022

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

farhadrclass and others added 2 commits October 15, 2022 15:48

Update src/utils.jl

49f1346

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

Update src/utils.jl

28e0a42

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

farhadrclass added 2 commits October 15, 2022 16:39

before finalizing the tests

5c3117b

updated the tests and the tutorial

7046022

paraynaud reviewed Oct 19, 2022

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Show resolved Hide resolved

src/KnetNLPModels.jl Outdated Show resolved Hide resolved

paraynaud reviewed Oct 19, 2022

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

farhadrclass and others added 2 commits October 19, 2022 11:56

Update Project.toml

82d7fc0

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

as requested by review

80c5766

farhadrclass requested a review from paraynaud October 21, 2022 14:56

paraynaud reviewed Oct 21, 2022

View reviewed changes

docs/src/tutorial.md Outdated Show resolved Hide resolved

Update docs/src/tutorial.md

969989a

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

paraynaud reviewed Oct 21, 2022

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

farhadrclass and others added 4 commits October 24, 2022 20:16

Update src/utils.jl

b5f0f58

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

Update src/utils.jl

2fcfde5

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

Update src/utils.jl

b208513

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

Update src/utils.jl

96e169a

Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>

farhadrclass requested a review from paraynaud October 25, 2022 00:17

paraynaud requested a review from dpo October 25, 2022 15:14

dpo reviewed Oct 28, 2022

View reviewed changes

farhadrclass and others added 5 commits October 28, 2022 14:48

Update docs/src/tutorial.md

fb96bd6

Co-authored-by: Dominique <dominique.orban@gmail.com>

updating minibatch

4573b50

Update src/utils.jl

8991130

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update utils.jl

e0c2adc

Merge branch 'PR_utilies' of https://github.com/farhadrclass/KnetNLPM…

0fe4aa7

…odels.jl into PR_utilies

farhadrclass added 2 commits October 28, 2022 15:26

Update utils.jl

57dd5a9

Update utils.jl

01ab710

no need for return 1 since our reset_minibatch_.. does reset them

dpo previously approved these changes Nov 4, 2022

View reviewed changes

paraynaud reviewed Nov 4, 2022

View reviewed changes

src/KnetNLPModels.jl Outdated Show resolved Hide resolved

paraynaud dismissed dpo’s stale review via c910dc1 November 4, 2022 13:12

paraynaud and others added 4 commits November 4, 2022 10:12

Update docs/src/tutorial.md

c910dc1

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update docs/src/tutorial.md

60ce04d

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update docs/src/tutorial.md

4b81294

Co-authored-by: Dominique <dominique.orban@gmail.com>

Update src/KnetNLPModels.jl

fabe70a

paraynaud self-requested a review November 4, 2022 14:31

paraynaud approved these changes Nov 4, 2022

View reviewed changes

dpo merged commit 2833c5d into JuliaSmoothOptimizers:main Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created a minibatch iterator selection function #57

Created a minibatch iterator selection function #57

farhadrclass commented Oct 14, 2022

paraynaud left a comment •

edited

Loading

farhadrclass commented Oct 15, 2022

paraynaud left a comment

farhadrclass commented Oct 21, 2022

paraynaud left a comment

paraynaud commented Oct 25, 2022

dpo Oct 28, 2022

farhadrclass Oct 28, 2022

dpo Nov 4, 2022

dpo Oct 28, 2022

farhadrclass Oct 28, 2022

dpo left a comment

dpo Nov 4, 2022

Created a minibatch iterator selection function #57

Created a minibatch iterator selection function #57

Conversation

farhadrclass commented Oct 14, 2022

paraynaud left a comment • edited Loading

Choose a reason for hiding this comment

farhadrclass commented Oct 15, 2022

paraynaud left a comment

Choose a reason for hiding this comment

farhadrclass commented Oct 21, 2022

paraynaud left a comment

Choose a reason for hiding this comment

paraynaud commented Oct 25, 2022

dpo Oct 28, 2022

Choose a reason for hiding this comment

farhadrclass Oct 28, 2022

Choose a reason for hiding this comment

dpo Nov 4, 2022

Choose a reason for hiding this comment

dpo Oct 28, 2022

Choose a reason for hiding this comment

farhadrclass Oct 28, 2022

Choose a reason for hiding this comment

dpo left a comment

Choose a reason for hiding this comment

dpo Nov 4, 2022

Choose a reason for hiding this comment

paraynaud left a comment •

edited

Loading