Outsource common NSTEPS code to function
#858
Conversation
schillic
force-pushed
the
schillic/NSTEPS
branch
from
0e3e3d0 to
3353c6a
Compare
|
i think this was the purpose of #734 |
src/Continuous/solve.jl
Outdated
| @@ -288,6 +288,17 @@ function _get_T(tspan::TimeInterval; check_zero::Bool=true, check_positive::Bool | |||
| return T | |||
| end | |||
|
|
|||
| function _get_nsteps(kwargs, δ, tspan) | |||
There was a problem hiding this comment.
we could do it similar to #860 (review) in the sense that on each algorithm we write
NSTEPS = get(kwargs, :NSTEPS, _get_nsteps(δ, tspan))
There was a problem hiding this comment.
Okay, feel free to make the change and merge.
There was a problem hiding this comment.
Actually, the new proposal is not equivalent: You always execute compute_nsteps (it is not done lazily), even if the user specified :NSTEPS. There are ways to store the function lazily and evaluate it only on demand, but maybe this is overkill and we just just use the previous version.
There was a problem hiding this comment.
hmm i dont follow this argument-- even if the function compute_nsteps is always executed because function arguments are executed, it is not mutating, so if NSTEPS was specified by the user, the result of compute_nsteps is discarded.
There was a problem hiding this comment.
ok, fair enough. it shoudn't fail :)
There was a problem hiding this comment.
So what do we do? Do you want to fix the test?
There was a problem hiding this comment.
i've proposed a change, but haven't tested it locally.
There was a problem hiding this comment.
Okay, let's see if that helps. I proposed a change because it should not return a float.
There was a problem hiding this comment.
It did not seem to help 😞
Does that mean there should be a separate module? |
|
i focused too much on the "Refactor init constants" and forgot the "to their own module" part :) unfortunately that issue doesn't have any associated description. we may as well just close it. |
mforets
force-pushed
the
schillic/NSTEPS
branch
from
3353c6a to
4c173a7
Compare
mforets
force-pushed
the
schillic/NSTEPS
branch
from
6f6178a to
3b7698e
Compare
No description provided.