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

Switch fabs for std::abs in new adaptive Adams-Bashforth-Moulton stepper #214

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

ds283
Copy link
Contributor

@ds283 ds283 commented Jul 25, 2017

I have been experimenting with the new adaptive Adams-Bashforth-Moulton stepper recently merged into odeint-v2. However, I've noticed that it doesn't play so well with floating-point value types other than double because some code in controlled_adams_bashforth_moulton.hpp, adaptive_adams_coefficients.hpp and pid_step_adjuster.hpp extract absolute values using fabs() without a namespace qualifier.

My understanding is that unqualified fabs() doesn't have overloads for types other than double (eg. http://www.cplusplus.com/reference/cmath/fabs/) — for example, at least on my platform there is already a problem at the level of using long double which clang warns will be implicitly narrowed to double. My understanding is that only std::fabs() or std::abs() have overloads for other types, and this seems to fit with the rest of odeint-v2 where std::abs() is normally used except for some parts of the vexcl interface.

This patch changes the uses of fabs() for std::abs() in just the three files mentioned at the top.

- controlled_adams_bashforth_moulton.hpp, adaptive_adams_coefficients.hpp and pid_step_adjuster.hpp extract absolute values using fabs() without a namespace qualifier

- if the integration value type is not double then this can cause problems, since fabs() is not required to have a long double overload. In such cases it is safer to use std::abs, and also this matches the rest of the odeint-v2 codebase
@mariomulansky
Copy link
Collaborator

You are correct, std::abs should be used.

@mariomulansky mariomulansky merged commit af59be4 into headmyshoulder:master Jul 25, 2017
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