-
Notifications
You must be signed in to change notification settings - Fork 6
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
Reformulate ARK #300
base: main
Are you sure you want to change the base?
Reformulate ARK #300
Conversation
save_func = cur_avg_err, | ||
kwargshandle = DiffEqBase.KeywordArgSilent, |
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 may have been an incorrect merge resolution: save_func
is not documented anywhere (it seems to be internal), so we shouldn't use it. Also, I don't like the idea of using DiffEqBase.KeywordArgSilent
.
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.
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.
The convergence tests were all failing with the callback solution on main. I agree that this should be done in terms of callbacks, though. Gabriele, maybe you could update this while ensuring that the convergence tests still pass?
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.
They were failing before this change. They've been periodically breaking on main for months. So, it's not clear to me that this needs to be reverted.
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.
In fact, I don't believe that these changes should change the results, so I'd be surprised if this was responsible for the docs breaking.
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 fixed all convergence tests in this rebase, and I was not able to get them running without reverting this. Granted, I didn't spend too much time debugging the callback version, so it might have been a simple issue.
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 would also be good to add @test
or @assert
calls to the convergence tests to avoid having them silently break for months.
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.
The problem is that it's flakey, it passes locally, or on push, and then fails on the main branch.
Another issue is that virtually nothing is printed when it errors, it's not clear which example is breaking, and what any of the types are. I think a better solution would be to run these examples on buildkite, and have a dependent job make a summary.
I don't want to add the use of internals back into this repo, so please fix the issue without save_func /KeywordArgSilent
, or fix the underlying issue, or leave it broken, as it has been (and very well may still be in this repo as the issue is not easily reproducible).
Work in progress
Closes #299