-
Couldn't load subscription status.
- Fork 9
Parametric meta #12
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
Parametric meta #12
Conversation
|
@tmigot I think I registered here : JuliaSmoothOptimizers/NLPModels.jl@e3b8b73 , will this fix your tests? |
Great, thank you! I will wait for the official release and then rebase. |
Codecov Report
@@ Coverage Diff @@
## master #12 +/- ##
==========================================
+ Coverage 97.92% 98.03% +0.11%
==========================================
Files 6 6
Lines 578 612 +34
==========================================
+ Hits 566 600 +34
Misses 12 12
Continue to review full report at Codecov.
|
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.
Thanks, that's a lot of work.
Most of my changes are stylistic and trying to minimize the number of changes.
I didn't review the last NLS parts because it's more of the same changes as NLP. After that, I'll review again.
src/feasibility-form-nls.jl
Outdated
| x0 = [meta.x0; zeros(nequ)], | ||
| lvar = [meta.lvar; fill(-Inf, nequ)], | ||
| uvar = [meta.uvar; fill(Inf, nequ)], | ||
| x0 = [meta.x0; zeros(T, nequ)], |
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 think this fails for GPU but I don't see a viable alternative, do you?
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 this work?
x0= zero(meta.x0, meta.nvar + nequ)
x0[1 : meta.nvar] .= meta.x0There 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 didn't test it, but this tutorial suggests that cat and fill are available methods for GPUArrays.
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.
Thanks. I want to approve but I'm scared of the Probot automerging again.
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.
LGTM besides my two comments.
@dpo I updated the constructors. What do you think? |
|
Thanks. And Probot auto-merged, but it was in accordance with what we discussed yesterday. |
Follows NLPModels.jl PR #353 with parametric meta.
Add tests on SimpleNLPModel and SimpleNLSModel for Float64 and Float32.