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.
Let prim functions return expressions #2190
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
Let prim functions return expressions #2190
Changes from all commits
cfb04b8
db03168
c1df65d
7eded17
0f85ba4
c92d5f7
26d745e
c08b000
4b830fc
3d13a2a
20eb3cd
14388eb
eb7a24f
1765307
46016f4
55bf3b2
873458c
a27e08e
a0091a2
c3dbb27
fa88821
0b15e7b
435a2b0
b2dbc29
c7ab13d
f84d6b2
b4d6208
62490e1
b316e1e
ccb1ad0
2dbe8eb
4575f35
4524299
257b6fd
a0523cc
bd33bbe
8dfd5b0
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.
Ahh, there must be more to the story. Do tell!
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.
Same for rev.
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.
And why the individual files?
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.
@syclik do you have any memories of why prim was originally included before
rev
andfwd
?prim
is the most general, so it makes sense to have it last here (see also this comment from this pull), but there could be something else I don't want to accidentally miss.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.
rev.hpp for example also includes prim, which we dont want included before fwd.
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.
I think it was the combination of these two:
https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-2190/15/pipeline
https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-2190/14/pipeline
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.
Oh and I see the conversation back here: #2190 (comment)
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.
Thqat can not really be an issue. If mix needs fwd or rev it is simply included. Same if fwd or rev needs prim.
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.
I don't remember what exactly was an issue, but it was something, that gcc allows even if it is not standard, while clang is more strict. I know I was reading this: https://clang.llvm.org/compatibility.html#dep_lookup
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.
Yeah I think this change is right.
If everything is template overloads, then we can define things after they are used. Like in this: https://godbolt.org/z/hbT54n
(you'll see both
test::myfunc
functions get called)If you add an overload of myfunc, it will only get called if the overload is already available when the compiler hits
whatever
. For instance, you can add this code above and belowwhatever
and see what happens:Since we're depending on overloading and templates in Stan math, we gotta make sure and include the overloads for the
rev
andfwd
types before the more generalprim
functions or the stuff inprim
might not be able to find everything.