-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Generalised unary vector function framework #1558
Merged
rok-cesnovar
merged 36 commits into
stan-dev:develop
from
andrjohns:feature/vec_gen_design
Jan 24, 2020
Merged
Changes from 19 commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
ca89f78
Initial implementation
de964ba
Merge branch 'develop' into feature/vec_gen_design
6b54c52
Add forwarding, rev & fwd versions
eebfd95
Merge branch 'develop' into feature/vec_gen_design
2b085d1
Add autodiff tests, remove arr versions
ac3d5e3
Nested testing
77f3a59
Fix tests, update doc
b3a1132
Tidy doc
27d1b1f
Merge branch 'develop' into feature/vec_gen_design
a4b83a7
Cpplint
d97c963
Tidy missing doc
461f0b0
log_softmax doc errors
cd0a362
Merge branch 'develop' into feature/vec_gen_design
9fa5124
[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…
stan-buildbot cba3a30
Fix failing test
e5a6b28
Merge develop
5a934fe
Revert head replacement
b7b2171
Merge branch 'develop' into feature/vec_gen_design
8ebea40
[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…
stan-buildbot a73be6d
Merge commit '426ad8fe5a2858b9d367aade1b25a631ac5e97e8' into merge_af…
rok-cesnovar 8b5cc7f
Merge commit 'd7eb73884e5fad18eaf323760e4625317e1c4c91' into merge_af…
rok-cesnovar df34056
Merge commit '2b2f7ddff32c12e1e253a6179bf81c1845962306' into merge_af…
rok-cesnovar 8a7017a
Merge commit '731b5f8cf6566db4f13a06851d56cc9e54029146' into merge_af…
rok-cesnovar 8214c93
Merge branch 'develop' into merge_after_flatten
rok-cesnovar d776eac
merge conflicts fix
rok-cesnovar 09c4004
[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…
stan-buildbot a0eb3df
fix header guard
rok-cesnovar 4e83afb
remove include
rok-cesnovar 2e4f6b1
Merge branch 'develop' into feature/vec_gen_design
febffbe
Address review comments
67e23b8
Merge branch 'develop' into feature/vec_gen_design
477cf9f
[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…
stan-buildbot 02afdb9
Fix merge error
8d8539a
cpplint
1fce5ac
Merge branch 'develop' into feature/vec_gen_design
f3b3286
Address comments
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Perfect forwarding brings no benefits here. This would work as well with no additional copys and the code is a bit shorter:
I think the same holds true for every other place in this PR that uses perfect forwarding.
Perfect forwarding is better than just const refs only if a variable can outlive the function in question. For example if the function can directley return the variable or construct some object that holds the variable.
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.
Can you link me to a doc that talks about that? My understanding of PF is that it tells the compiler "I'm okay with moving ownership of this memory and leaving the original object in an undefined state"
In particular, for the below think about a recursive function that can have a long ref stack that the compiler probably can't sort through. PF gives the compiler the ability to move ownership of that memory. So we don't have to worry about whether the compiler will have alias issues when deciding to inline recurses.
The CPP guides have a lot on this with a nice little table in F.15 has some good heuristics
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing
In particular this seems to recommend Andrew keep it as a forwarding reference
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f19-for-forward-parameters-pass-by-tp-and-only-stdforward-the-parameter
It feels like the above func is just something we want to allow the compiler to move through if it's able
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.
Sorry, that is just my understanding and I a quick search does not find anything on when perfect forwarding is benefitial and when it is not. I can't think of any other example, but please correct me if I am wrong.
I would say more accurate would be "I am CAPABLE of either moving a temporary object or using a reference to lvalue object." In this PR no function can actually move the parameter, so const refs are just as good.
I don't know anything about that. Can you link any doc or benchmark that supports this?
Thanks for the links to the guidelines. However I disagree with your conclusion that they suggest we should use perfect forwarding. The first link points to section that says simple means of parameter passing (which exclude perfect forwarding) should be used, unless benefits of something more advanced are demonstrated.
The second link suggests nothing on whether perfect forwarding should be used or not. It only explains what a function accepting a forwarding reference should do with it. It says that a function should be flagged (as violatin those guidelines) if it does anything with this argument except
std::forward
-ing it exactley once.I would add to this that
std::forward
-ing an argument to function is only beneficial if that function either uses perfect forwarding or has two overloads accepting const lvalue ref and rvalue ref.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 agree that we shouldn't clutter the code with perfect forwarding examples where they can't be used. This came up in another PR with autodiff types, which have no data content to move.
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.
@t4c1 at work but ill link some stuff tonight
Maybe we should write up an rfc to talk about what types and sizes should use certain argument types. I tend to be way to pf giddy