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

Online edits #63

Merged
merged 16 commits into from
Jun 23, 2023
Merged

Conversation

farhadrclass
Copy link
Contributor

The first round,
I am adding my edits and notes into next push

@farhadrclass
Copy link
Contributor Author

@paraynaud
one more push is to come soon to have FLuxNLP example

@paraynaud
Copy link
Member

paraynaud commented Jun 20, 2023

@farhadrclass I've allowed the generation of the .pdf (at least for your last commits).
You can take a look at the final result in details of the JOSS/.... job, then click on summary and look at the artifact !

@farhadrclass
Copy link
Contributor Author

@dpo @paraynaud I added the examples of the FluxNLPModels with R2

Copy link
Member

@paraynaud paraynaud left a comment

Choose a reason for hiding this comment

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

Thanks for the work @farhadrclass.

Defining a neural network is a lot easier with Flux.
We should put the Knet.jl architecture and KnetNLPModel initialization into Annex, and make sure the code running for the remaining FluxNLPModel works also for the KnetNLPModel in Annex.

paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
farhadrclass and others added 6 commits June 21, 2023 12:43
Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>
Co-authored-by: Paul Raynaud <paul.raynaud66@hotmail.fr>
@farhadrclass
Copy link
Contributor Author

@paraynaud ready for review

@farhadrclass
Copy link
Contributor Author

@farhadrclass I've allowed the generation of the .pdf (at least for your last commits). You can take a look at the final result in details of the JOSS/.... job, then click on summary and look at the artifact !

I can not find this

paper/paper.md Outdated Show resolved Hide resolved
paper/paper.md Outdated Show resolved Hide resolved
@farhadrclass
Copy link
Contributor Author

farhadrclass commented Jun 23, 2023

This is from: farhadrclass#1

Straightforward pass onto Summary + Statement of need + KnetNLPModel example.

What is left to do (beside FluxNLPModel example), e.g. comment not answered by the changes :

  • mention : "It is important to stress that we want to be able to experiment with solvers that enforce a decrease of the merit function";
  • answer : "Why not just write solvers directly in Flux.jl or Knet.jl";
  • mention callback : but it appears in the example section.

After rebasing onto PR-JOSS-Flux and PR-JOSS:

  • move Knet model definition to the end to put Flux architecture on the front (with a mention to the Knet model);
  • make sure the code calling solver works for both models (same instruction, same callback ...)

@farhadrclass
Copy link
Contributor Author

I added:

  1. Example from FluxNLPModels
    1.1 Add accuracy to FluxNLPModels
    1.2 Added MinibatchNext to FluxNLPModels
    1.3 Simplify and clean the example
  2. Write the example in Markdown and edit
  3. Move the text (Flux before Knet)
  4. Edit the first section
  5. Edit the text overall
  6. Apply @dpo remarks
  7. Answer Why not just write solvers directly in Flux.jl or Knet.jl"; but I am not 100% sure if that is enough

@paraynaud please review and let me know what you think?

@paraynaud
Copy link
Member

Nice work, thanks!

@paraynaud paraynaud merged commit 35a3a92 into JuliaSmoothOptimizers:PR-JOSS Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants