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
Generic var templates for operators and std::iterator_trait var/fvar specialization #1525
Generic var templates for operators and std::iterator_trait var/fvar specialization #1525
Changes from 12 commits
8309363
bd9b20c
f8f2e3a
8806331
a6b4d81
3868acf
5e07ef9
119a7e7
1348dda
858e1f2
ec6345a
4300497
fd35443
73181b4
36777ab
ad62f4f
fd2a4a8
930329d
6185370
12ec611
88c86e8
5564039
a694d1a
5740cd4
4cba532
30ee8c9
1a565b6
75eaf43
1342d33
3604151
9eebaa7
993ceaa
cd60021
ced2df7
8d460ac
2795719
99a916b
84f5b26
06ad089
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.
[comment]
These are all super useful. Thanks for adding.
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'd strongly prefer
removing the
require_
prefix because that's implying a use, not a behaviorreplace
_t
with_v
because it denotes a boolean value, not a type; this follows the behavior ofis_arithmetic_v
;enable_if_t
ends in_t
because it denotes a type (void
by default), not a value.For example, that'd be
not_autodiff_v<T>
fortrue
ifT
is not an autodiff type, and usage would be, for exampleenable_if_t<not_autodiff_v<T>, U>
.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 clarify? I'm not sure i understand how the require part of the name is implying a use over a behaviour. The intent with the
require_
in the name is because all thoserequire_*
s are trying to emulate a legacy version of C++20 concepts. It's for if you want a universal reference in the function signature for all the weird value types and const combinations. In terms of use/behavior I think my answer is that I think about C++ concepts like compile time contracts (which feels like a behaviour, though idk what that word means in context here)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
require_*_t
metaprogramming stuff is essentially a convoluted / compact way to writestd::enable_if_t<...>
i.e. they also returnvoid
.You can think of
require_autodiff_t
as an alias forenable_if_t<not_autodiff_v<T>, U>
.Also idt we can use C++ compile time constexpr objects with c++1y (anything _v like how std does) since the windows compiler we require compatibility with for Rtools has a bug for those
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 misunderstood and thought this was a boolean. Now that I see what it's doing, I still think it needs to be renamed. How about
enable_if_autodiff_t
to enable if it's an autodiff type? That follows the standard library naming conventions more closely thanrequire_autodiff_t
.We can then use
disable_if_autodiff_t
instead ofenable_if_not_autodiff_t
.We can build similar ones for primitives,
enable_if_arithmetic_t
anddisable_if_arithmetic_t
.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.
Should these all have doc or are they so close to the ones without
_v
that they're obvious? cppreference actually doesn't doc these and they doc everything else meticulously.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've been throwing this back and forth with myself. I've leaned towards not doc since aliases are inlined in the docs you can see the full definition
http://mc-stan.org/math/d0/dfb/group__require__base__types.html#gac55eae5c1fc2f38a96336b5f0bc94449
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.
This is only a few things from
<cmath>
---the rest of the functions defined in that header follow the Stan convention of being broken into their own files. I don't think we want to stray from that convention here. Sorry for all the extra work---it's one of the reasons I was dreading breaking this all out into tests and definitions.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.
looking at cmath it looks like all these are in the cmath header? For clarification you want
signbit
,isinf
,isfinite
,isnan
,copysign
, andisnormal
to be in their own header 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.
Can we write this with
value_of_rec(v)
and make it generic across our autodiff types? Or havit recurse withv.val()
if both forward-mode and reverse-mode support.val()
. Either way, it can go inprim
as it's not mentioning any autodiff types itself.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 since fvar and var have
.val()
we can move this toprim
. Should we also have each of these for arithmetic types or just rely on the cmath impl for those (in case someone callsstan::math::signbit(a)
where a is double)?If we wanted to be v smart we could break these up to have signatures for
.val()
.val()
(i.e.complex<>
/fvar<var>
/fvar<fvar<var>>
)Then 1 and 2 would pass by copy while 3 would be able to forward along / pass a reference to
.val()
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.
This code comment is redundant given the API doc (and apoloties for putting it there originally).
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.
If
require_all_var_t
requiresVar1 == var
andVar2 == var
, why not just use theconst var&&
arguments?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 direct answer to this is that a function signature with
const var&&
would only accept constant rvalue types. TheVar1&&
andVar2&&
in the function signature are universal references. Combined withrequire_all_var_t
the operators here acceptvar&
,const var&
, andvar&&
(they could acceptconst var&&
though idt const rvalues are a thing). It allows the caller (callee?) to do something likeauto the_sign = copysign(std::forward<FVar>(an_fvar));
to move a fvar, for example, when the call site can tella_fvar
is movable.The longer indirect answer is that if we want "universal references" in function signatures for perfect forwarding then you can't have any declaration specifiers alongside the type specifier for each function parameter.
Would we ever allow a
var
to be moved?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 is different between
require_var_t<Var>
and just removing the template parameter and usingvar
instead ofVar
? I'd think given that it's an overload, we'd also prefer the more specific type to match.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.
As with the comment above about universal references the
Var&&
andrequire_var_t
combo here will match against anyvar
type with whatever deceleration specifiers it has on it (const etc.). So it gives us the most specific type as long as the paremeters decayed type is avar
.Does that make sense? ftr I find the universal reference thing in C++the most weird/legalize part of the language
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.
Similar comment/request for
var
as before. I'm going to stop pointing these out as the pattern seems to be used everywhere.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.
[optional]
The right doc for
a
isdividend
, forb
isdivisor
, and for the result isquotient
.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.
Word!