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

warning if dt is modified during kernel returning SUCCESS #657

Merged
merged 12 commits into from
Oct 4, 2019

Conversation

delandmeterp
Copy link
Contributor

This PR captures if a kernel has modified particle.dt.
This leads to problems in many situations:

  • In our code, particle.time will be updated using the dt before the kernel call, ignoring the dt, which will be used at next kernel call (so one time step later)
  • everything called after the particle.dt will then be executed with the wrong dt

-> There is no problem if after modifying dt, the kernel returns REPEAT, in which case, all variables are reset to their value before the kernel call (except dt) and the kernel is rerun

Problems:

  • In current version of the PR, JIT particles call a warning for each occurence of p.dt modification. This can lead to a lot of prints
  • the test does not capture the scipy particle warning. Nothing is really tested there
  • Should JIT particle return a different error code in case of p.dt modifications, and a true warning_once be printed? There will be only one print, but the C loop will be broken all the time...

Solution? Why do we want to raise a warning? Why don't deleting the particle (or even killing the run?). Modifying dt during kernel execution is dangerous. Some users could use such operation correctly. But they could also do the same job without modifying the dt like this. It's better to completely disable such operation

@delandmeterp
Copy link
Contributor Author

So we decided that the code should crash if dt is modified without REPEAT error.
This makes sense.

Now, we have a problem with RK45. RK45 does either divide dt by 2 if accuracy is bad, or multiply it by 2 if accuracy really good. We were not repeating the kernel in the last case (which was wrong).

But if RK45 requests to multiply dt by 2, but dt can't be increased since t+dt > tend, them we are stuck in an infinite loop.

@delandmeterp
Copy link
Contributor Author

Solution might be the following:

We wanted to do "if dt is modified, code crashes"
Why not: "if dt is modified*, then kernel.repeat"

by modified*, I mean modified between "before kernel call" and "after kernel call + adjustment due to tend"

I think this would make sense

@erikvansebille
Copy link
Member

I think this would make sense

Yes, I agree.

@delandmeterp
Copy link
Contributor Author

Limitation is that if kernel does particle.dt *=2 then it loops forever, but
(1) it would have crashed anyway with first suggestion
(2) we already have possible infinite loops in current code. So it's a different weakness

I'll implement this then.

@@ -275,8 +275,11 @@ def execute_python(self, pset, endtime, dt):
for var in ptype.variables:
p_var_back[var.name] = getattr(p, var.name)
try:
p.dt = sign_dt * dt_pos
pdt = sign_dt * dt_pos
p.dt = pdt
Copy link
Member

Choose a reason for hiding this comment

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

These two lines are somewhat unclear. Would it help to rename pdt to something clearer, e.g.

p.dt =  sign_dt * dt_pos
pdt_prekernels = p.dt

and then check ... not np.isclose(p.dt, pdt_prekernels) below?

@delandmeterp delandmeterp merged commit 55e047f into master Oct 4, 2019
@delandmeterp delandmeterp deleted the warning_if_modified_dt branch October 4, 2019 10:16
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.

2 participants