-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
Bump compats and update tutorials for Optimization v4 #950
Conversation
This is stalled and will likely lead to more people complaining about the Optimization change can we get a decision for it? |
@avik-pal update Boltz? |
Some tests still fail for DataInterpolations, let me dev and patch those shouldn't be hard. |
display(l) | ||
global iter | ||
iter += 1 | ||
if doplot && iter % 1 == 0 | ||
# plot the original data | ||
plt = scatter(tsteps, ode_data[1, :]; label = "Data") | ||
|
||
# plot the different predictions for individual shoot | ||
plot_multiple_shoot(plt, preds, group_size) |
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 don't see how this is going to work if we don't have the predictions?
println(l) | ||
# plot current prediction against data | ||
if doplot | ||
pred = predict_neuralode(state.u) |
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.
This is not a viable solution, it doubles the cost. We should show a better standard way to do this.
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 was the same before the gradient and loss were evaluated separately so I figured it's a fair tradeoff, the current alternative is to use a global variable.
Down the line I plan to improve OptimizationState so it can be hacked for these things, I am not sure how it should be yet but should be possible
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 show a current best practice that does not involve recomputing since that can be quite a regression.
What's the state of this? |
70c4fb3
to
d20f849
Compare
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Yay! 🎉 |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.