-
Notifications
You must be signed in to change notification settings - Fork 130
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
SUNStepper basics based on MRIStepInnerStepper #463
Conversation
@drreynolds @gardner48 I have made the changes we discussed on Tuesday. Let me know what you think. |
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 like how clean this is now. I noticed a few items for comment (see below).
a030d57
to
c04f24a
Compare
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.
Initial pass at latest changes
After some more thought and discussion with David on IDA and passing around I'd like to propose the following to make SunStepper self starting (all you need is
where there's a function which can be specified by users (we could have a default that uses |
I actually need the OneStep function in the adjoint work, so I have added it back into here. |
It looks like the only remaining issue with the CI is that the documentation updates have some label errors:
|
Yeah, wasn't sure how to best handle that. Those are references to parts of the docs that will be added in the operator splitting PR (which is merged after this). I can temporarily comment out or remove the references out to get the CI passing. |
I forget, but is the "merge" plan to approve/merge this PR before #572? If that's the case, then perhaps we should move these bits of documentation into #572 so that both "squashed" and merged PRs can pass all CI tests? |
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.
One minor change (missing process error call) that I'll apply and approve in a sec.
I am opening this PR as a draft to gather feedback. This renames/moves
MRIStepInnerStepper
toSUNStepper
so that @Steven-Roberts and I can extend it for other purposes.