Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 missing scalar prim signatures #2612
Add missing scalar prim signatures #2612
Changes from 14 commits
103cc03
a4ddd66
3411c67
e6d68e8
4075c09
af70d36
872803c
969d378
64fa890
02bee11
d7af743
dd142c3
228a05f
43c3a99
5769a7b
50e54f7
e60adb1
b3f9ff3
dc23f52
8f730bb
2301ba1
0f4383d
465a551
4a10aee
00397e9
79d0351
d9795d2
ed76fdd
e13933d
0925494
382be99
aac0a55
199da11
27b3748
943ccd3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this return an integer if
x
andy
are both integers? Usually, the minimum promotion target we've used isdouble
. If not, you have to be careful withstd::min
because it takes a single template class parameter which won't be matched if one argument'sint
and the otherdouble
. You can get around that callingfmin
, but that won't returnint
types forint
arguments. Or you can just have an overload for(int, int)
args, and I think it'd be more specific than this template.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will. That is tested here: https://github.com/stan-dev/math/blob/develop/test/unit/math/mix/meta/return_type_test.cpp#L17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for not just writing unit tests for these? These tests are all very much boilerplate, but I don't see the need for moving to Python. One of the reasons I prefer basic tests is that they're easy to find in the code. With this kind of thing, I'll never be able to find where
math(int, double)
gets tested. And these should all be tested on a mix of integer and real inputs to make sure promotion and return type calculation works as expected.And where are we testing that they produce the same values as the C++ functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decision to make these tests automatic is mostly because I wanted to test that all scalar argument functions work correctly.
But I guess we only need to test std:: shadowing functions, which is a non-changing small list of functions. Will revert to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result_1
andresult_2
have the same definition. Should one of these have a different arg list?