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

Add relative error check in adaptive time stepping algorithm for rk23… #275

Open
wants to merge 1 commit into
base: 3.11
Choose a base branch
from

Conversation

xfong
Copy link

@xfong xfong commented Nov 18, 2020

…, rk45dp and rk56 time steppers

@godsic godsic requested a review from JeroenMulkers November 18, 2020 08:05
@godsic
Copy link
Contributor

godsic commented Nov 18, 2020

@xfong

First of all, that you very much for the effort and for sharing this one with us.

Could you please

  • elaborate a bit on motivation for using relative error control;
  • provide some example .mx3 scripts where it helps overcome absolute error control limitations;
  • comment on what to expect performance- and precision-wise.

@xfong
Copy link
Author

xfong commented Nov 18, 2020 via email

@xfong xfong force-pushed the kelvin_stepper_update branch 2 times, most recently from d9bf196 to 1b1eaee Compare November 18, 2020 09:59
@godsic godsic added this to the 3.11 milestone Nov 18, 2020
@godsic
Copy link
Contributor

godsic commented Nov 18, 2020

@xfong

  • please be prepared for a long review process as this pull request touches very sensitive part of mumax3;
  • please restructure your code in a way, that using RelErr = 0 exactly follows the current code, while RelErr > 0 triggers your modifications.

@xfong xfong force-pushed the kelvin_stepper_update branch from f41d857 to 5c92e01 Compare November 18, 2020 11:33
@xfong
Copy link
Author

xfong commented Nov 18, 2020 via email

@xfong xfong force-pushed the kelvin_stepper_update branch from 5c92e01 to c8c3e9e Compare August 26, 2021 01:46
@xfong xfong force-pushed the kelvin_stepper_update branch from c8c3e9e to 1debc67 Compare December 15, 2021 04:39
@xfong xfong force-pushed the kelvin_stepper_update branch 2 times, most recently from 479e821 to 48b0c2b Compare June 6, 2022 01:36
@xfong xfong force-pushed the kelvin_stepper_update branch from 48b0c2b to 423b316 Compare January 3, 2023 02:53
if fail == 0 || RelErr <= 0.0 || rlerr < RelErr || Dt_si <= MinDt || FixDt != 0 { // mindt check to avoid infinite loop
// step OK
setLastErr(err)
setMaxTorque(k2)
Copy link
Contributor

@JonathanMaes JonathanMaes Oct 2, 2024

Choose a reason for hiding this comment

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

It seems that the value used in setMaxTorque() is inconsistent between different solvers. [NOTE: this was already the case before this PR]

  • Here in RK56 (also RK45dp and RK23), setMaxTorque() is set with the torques acting on the final magnetization state (i.e. the torqueFn() values after the final cuda.Madd<n>(m, m0, ...) is performed).
  • However, in e.g. RK4 (also Heun and Euler I think?), setMaxTorque() is set with the torque acting on the magnetization considered in the last stage of the RK solver (i.e. the torqueFn() values before the final cuda.Madd<n>(m, m0, ...) is performed).

Which of these two possibilities do we consider most correct?

EDIT: the reason for not calculating the final torque in RK4 etc. is that this would significantly decrease performance for simulations at nonzero temperature because then this final torque can not be re-used. Therefore, it seems that the final torque evaluation of k2 in the original RK56 code could be omitted similar to how it was done in the RK4 solver.

@@ -39,73 +50,105 @@ func (rk *RK56) Step() {
h := float32(Dt_si * GammaLL) // internal time step = Dt * gammaLL

// stage 1
torqueFn(k1)
torqueFn(rk.k1)
Copy link
Contributor

@JonathanMaes JonathanMaes Oct 2, 2024

Choose a reason for hiding this comment

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

There is no need to re-calculate this (at zero temperature, at least) if k2 is stored at the end of the Step anyway.

@JonathanMaes
Copy link
Contributor

Hi, thanks for your contribution!

@xfong Could you elaborate a bit on how your code works?

You claim that it can provide a >5% performance increase, but as far as I understand your code, an extra if statement (checking the relative error) was added in the part of the original if statements where the step was accepted. If that check fails, the solver undoes the step anyway. Hence, it seems to me that your extra if statements provide an extra pathway to undo the step, so I fail to see how this can increase performance. Am I missing something here?

Would using a total tolerance (something like tol = AbsTol + RelTol*torque) be an option, which combines the absolute and relative tolerances into a single check?

@JonathanMaes JonathanMaes removed this from the 3.11 milestone Oct 3, 2024
@JonathanMaes JonathanMaes changed the base branch from master to 3.11 October 4, 2024 15:08
@xfong
Copy link
Author

xfong commented Oct 10, 2024

The AbsTol imposes a very strict check especially in steps with large changes in magnetization. The relative tolerance check allows the step to be accepted as long as the error is small relative to the change in magnetization. It is this that enables larger steps to be accepted by the steppers, which then leads to the roughly 5% faster runtimes across all the transient simulations found in the ./test directory.

Hi, thanks for your contribution!

@xfong Could you elaborate a bit on how your code works?

You claim that it can provide a >5% performance increase, but as far as I understand your code, an extra if statement (checking the relative error) was added in the part of the original if statements where the step was accepted. If that check fails, the solver undoes the step anyway. Hence, it seems to me that your extra if statements provide an extra pathway to undo the step, so I fail to see how this can increase performance. Am I missing something here?

Would using a total tolerance (something like tol = AbsTol + RelTol*torque) be an option, which combines the absolute and relative tolerances into a single check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants