Skip to content

Cleanup unnecessary prim/fun signatures and checks (redundant as of C++11) #1702

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

Closed
wants to merge 7 commits into from

Conversation

rok-cesnovar
Copy link
Member

@rok-cesnovar rok-cesnovar commented Feb 12, 2020

Summary

C++11 added overloads for integers for some of the base functions. Previously Stan Math had its own overloads for these to avoid ambiguity with (var) overloads as it wasnt clear whether to promote to double or var.

This PR removes the following:
- asinh(double) and asinh(int)
- cbrt(double), cbrt(int)
- erf(double), erf(int)
- erfc(double), erf(int)
- exp(int)
- expm1(int)
- log(int)
- sqrt(int)
- round(double), round(int)
- trunc(double), trunc(int)

The following integer overloads will stay in:

  • acosh, atanh, log1p (I cleaned up these a bit because I dont think is_nan on integer values is needed). The overloads have to stay because Stan Math throws for some input values, std:: returns nan and std:: throws for value we want to throw ourselves.
  • lgamma (no changes here, this will stay because std::lgamma is not thread-safe)

Due to removing some signatures I had to fix the functions to now use std for double/int inputs. And that then led to a minor code cleanup in gp_matern* files.

This now also fixes #1704 by including cmath and adding using std::log statements in lgamma_stirling and lbeta.

Tests

No new tests, existing tests cover all the cases.

Side Effects

Hopefully none.

Checklist

@rok-cesnovar rok-cesnovar changed the title clean up unnecessary signatures and checks Cleanup unnecessary prim/fun signatures and checks (redundant as of C++11) Feb 12, 2020
@mcol mcol linked an issue Feb 12, 2020 that may be closed by this pull request
@rok-cesnovar rok-cesnovar linked an issue Feb 12, 2020 that may be closed by this pull request
#ifdef _WIN32
if (is_inf(x))
return x;
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

This worked fine on my Windows box, but will let Jenkins give the final verdict. Will just revert back if Jenkins flags this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what made the test fail on Jenkins?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will remove once I update the PR. Was hoping it could go away :)

#endif
return std::acosh(x);
}
check_greater_or_equal("acosh", "x", x, 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

I think is_nan for integer inputs is redundant, isnt it?

@rok-cesnovar
Copy link
Member Author

Closing this for now as there are some additional question that need to be solved.

@rok-cesnovar rok-cesnovar deleted the cleanup_int_overloads branch March 19, 2020 16:44
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.

Scalar lgamma_stirling and lbeta truncate log input to integer Investigate which int overloads are still needed (exp, pow, ...)
3 participants