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

Reformulate ARK #300

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Reformulate ARK #300

wants to merge 3 commits into from

Conversation

Sbozzolo
Copy link
Member

Work in progress

Closes #299

Comment on lines +210 to +211
save_func = cur_avg_err,
kwargshandle = DiffEqBase.KeywordArgSilent,
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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).

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.

Move SSPKnoth to reformulated ARK framwork
3 participants