-
Notifications
You must be signed in to change notification settings - Fork 10
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
Steady state callback #601
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
==========================================
- Coverage 69.92% 69.08% -0.84%
==========================================
Files 86 87 +1
Lines 5120 5185 +65
==========================================
+ Hits 3580 3582 +2
- Misses 1540 1603 +63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 is one mistake in the condition since you pop/push the current ekin one value is always 0.0 which will effectively reduce the interval size by one.
@@ -0,0 +1,175 @@ | |||
""" | |||
SteadyStateCallback(; interval::Integer=0, dt=0.0, interval_size::Integer=10, |
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 find the name a bit misleading. In grid-based methods, when simulating a physical instability arising from a slightly perturbed steady state, you sometimes subtract the right-hand side of the steady state to remove errors.
From this name, I would expect something like this here.
I'll think about other naming suggestions.
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.
ConvergenceConditionCallBack
ConvergenceTerminationCallBack
TerminationConditionCallBack
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.
SteadyStateDetector
SteadyStateTerminator
# `terminate!(integrator)` terminates the simulation immediately and might cause an error message. | ||
integrator.opts.maxiters = integrator.iter |
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 causes a warning message.
A more elegant solution would be to initially (in initialize!
) sort the CallbackSet
in such a way that the SteadyStateCallback
is last.
The set is stored in integrator.opts.callback
. However, I have no idea how to check if the actual callback is the last element or not.
Any idea @efaulhaber and @svchb ?
edit: The reason for this is the following.
CallbackSet(some_callback, steady_state_callback)
is totally fineCallbackSet(steady_state_callback, some_callback)
causes an error message when terminating.ERROR: MethodError: reducing over an empty collection is not allowed; consider supplying
initto the reducer
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.
Another (simpler) solution:
@suppress integrator.opts.maxiters = integrator.iter
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 guess the only way would be to implement a custom solve function which wraps the ODE solve?
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.
My prefered approach is something like:
function initialize_cb!(cb::DiscreteCallback{<:Any,
<:PeriodicCallbackAffect{<:SteadyStateCallback}},
u, t, integrator)
callback_set = integrator.opts.callback.discrete_callbacks
last(callback_set) isa typeof(cb) && return nothing
# Make sure that the last callback is a `SteadyStateCallback` to avoid an error message when terminating
new_set = (filter(i -> !(i isa typeof(cb), callback_set))...,
filter(i -> i isa typeof(cb), callback_set)...)
integrator.opts.callback.discrete_callbacks = new_set
return nothing
end
However, I think we can't just mutate the CallbackSet
.
@efaulhaber what's your opinion?
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.
Otherwise a solution would be to adjust the CallBacks upstream?
I tested the callback with the
pipe_flow_2d.jl
example. It terminates after~0.8sec
.I consider the change in kinetic energy in a
0.002*10
interval.