Skip to content

Conversation

@ChrisRackauckas
Copy link
Member

@ChrisRackauckas ChrisRackauckas merged commit 4d6ead4 into master Jan 12, 2021
@ChrisRackauckas ChrisRackauckas deleted the zero_dt branch January 12, 2021 07:13

# Evaluate condition slightly in future
abst = integrator.tprev+integrator.tdir*max(abs(integrator.dt/10000),100*eps(integrator.t))
if integrator.t == 0
Copy link
Member

Choose a reason for hiding this comment

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

Why test for zero here specifically and not the initial time point? And it seems maybe the problem could also be solved without this branch by specifying a smaller dt?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is due to eps(integrator.t) at t==0 being absurdly small.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I got the impression the problem was that abst was too large before from the discussion on discourse. Since you removed one part of the max call the new value should be less than or equal to the old value - but if it the eps part was too small it should actually be the same, shouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only reason that other part is there is because of you use eps(integrator.t) at zero you get:

julia> eps(0.0)
5.0e-324

which isn't helpful. At all other spots it's adapted to the relative size of the values in the problem, so it's a good estimate. So that max was there to cover that other case. But it seems like integrator.dt/10000 ended up dominating the max even when it shouldn't, so I've relegated it to exactly the one spot where it matters.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh now I understand, thanks!

@klaff
Copy link

klaff commented Jan 13, 2021

Pardon my ignorance, but is this the right way to pick up the changes in master branch of DiffEqBase?

begin # define independent package environment
	import Pkg
	Pkg.activate(mktempdir())
	Pkg.add("Plots"); using Plots
	Pkg.add(name="DiffEqBase",rev="master");
	Pkg.add(name="DifferentialEquations"); using DifferentialEquations
	Pkg.add("PlutoUI"); using PlutoUI
end

I'm not seeing a change in the behavior.

@ChrisRackauckas
Copy link
Member Author

I just tagged. Get the new version.

@klaff
Copy link

klaff commented Jan 14, 2021

Much better on the bouncing ball. I still have a similar failure in my "real" simulation which uses VectorContinuousCallback. I haven't found an error in my code yet but it's much more complicated so not a good MWE.

EDIT: The bouncing ball MWE rewritten with VectorContinuousCallback also works correctly (by which I mean it doesn't miss the cross until the time between bounces approaches eps(t)).

@klaff
Copy link

klaff commented Jan 14, 2021

Hi Chris, I have another example which still fails with DiffEqBase = v6.53.5. Could still be my error but I've stared at it too long and need some other eyes on it. I would also like to discuss whether I'm approaching this in a reasonable fashion or am I trying to use a screwdriver as a hammer.

Where would be best to post this example? (Here, new issue, new discourse thread, ?) Thanks.

@ChrisRackauckas
Copy link
Member Author

New issue.

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.

4 participants