Expanding 5 macros STAN_ADD_REQUIRE_* directly into code.
#2965
Conversation
|
The way these were expanded deletes all the docs for requires from the stan math website |
|
Yes. I know.
They can come back.
I’m looking at the next level of work right now which is to reduce the
footprint of these requires. There’s no need to have the complete set if
we’re not using most of it.
We can close this PR after it goes through tests. I wanted to make sure it
actually works as intended first.
…On Mon, Oct 23, 2023 at 10:56 AM Steve Bronder ***@***.***> wrote:
The way these were expanded deletes all the docs for requires from the
stan math website
https://mc-stan.org/math/group__rev__row__vector__types_ga04400fc27cdcef7938e1a79d14068a7b.html#ga04400fc27cdcef7938e1a79d14068a7b
—
Reply to this email directly, view it on GitHub
<#2965 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADH6FYJJXT5NCORSU7GBG3YA2ARVAVCNFSM6AAAAAA6KTCYHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZVGM4TINJXHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
imo if we want to go this route I'd first see how many requires we can delete. Is this all the requires folded out? adding ~5k lines is not the worst, but it is a lot of replicated code over and over and I worry about the maintenance of it. |
|
Yes, absolutely everything taken out of the macros! I’m on another branch
seeing what can be removed.
I completely agree, but I think the actual footprint is going to be much
less. Enough so that we can write real doc and add some tests for the
things we actually use.
I submitted this as a PR to kick off testing. Since it’s so much stuff, I
don’t trust “works on my machine.”
I’ll tag you once I knock out the unused requires. Let’s see how that
looks.
…On Mon, Oct 23, 2023 at 11:07 AM Steve Bronder ***@***.***> wrote:
imo if we want to go this route I'd first see how many requires we can
delete. Is this all the requires folded out? adding ~5k lines is not the
worst, but it is a lot of replicated code over and over and I worry about
the maintenance of it.
—
Reply to this email directly, view it on GitHub
<#2965 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADH6F2UZBWGCHG4WBASMGDYA2B4DAVCNFSM6AAAAAA6KTCYHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZVGQYTSMJZGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@SteveBronder, I just walked through all 1108 |
syclik
force-pushed
the
feature/simplify-meta
branch
2 times, most recently
from
faa6a03 to
e540cba
Compare
|
@SteveBronder, I think the doc in Mind taking a look? If you have any suggestions on how to make that doc better, happy to give it a try. |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Co-authored-by: Steve Bronder <[email protected]>
Co-authored-by: Steve Bronder <[email protected]>
syclik
force-pushed
the
feature/simplify-meta
branch
from
e41ec3c to
a1d05d9
Compare
|
@SteveBronder, thank you for the detailed review!
|
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@SteveBronder, anything else we need to change on this PR before merging? |
|
Let me look this over one more time but I think it is good! Should this get merged into the 5.0 branch since we are getting rid of a lot of the requires? If so then I think we should wait till the other ones are merge into the 5.0 branch as this will probably be the biggest diff |
| In the Math library, we use a technique that allows the definition of | ||
| multiple template functions where each handles a subset of allowable | ||
| types. We add a pointer [non-type template | ||
| parameter](https://en.cppreference.com/w/cpp/language/template_parameters#Non-type_template_parameter) | ||
| to the template parameter list with a default value of `nullptr`. Any | ||
| `void*` non-type template parameter with a default of `nullptr` is | ||
| valid and the non-type template parameter is ignored by the | ||
| compiler. Utilizing [substitution failure is not an error | ||
| (SFNIAE)](https://en.cppreference.com/w/cpp/language/sfinae), a | ||
| substitution failure for the non-type template parameter will result | ||
| in that definition being removed from the possible function | ||
| definitions. Using this technique we have to be careful not to violate | ||
| the [One Definition | ||
| Rule](https://en.cppreference.com/w/cpp/language/definition) and only | ||
| provide one definition for any set of types. |
There was a problem hiding this comment.
How about something like the below instead? I think it would be good to flesh out the explanation of how this works more like in the below
In the Math library, we use a technique similar to C++20's require keyword that allows the definition of
multiple template functions where each handles a subset of allowable
types.
When the compiler attempts to resolve which function should be called from a set of templated function signatures there must be only one possibly valid function signature available. This is called the One Definition
Rule. For example, the following code would fail because the compiler is unable to differentiate between the two function signatures.
template <typename T>
T foo(T x) {
return x;
}
template <typename K>
K foo(K x) {
return x;
}The compiler needs a way to differentiate between the two signatures to select one and satisfy the One Definition Rule. One trick to have a single valid definition is to utilize Substitution Failure Is Not An Error
(SFNIAE) to purposefully create conditions where only one signature is valid because all of the other conditions fail to compile. The simplest way to do this is to start with a type trait like the below enable_if. The enable_if is only defined for the case where B is true and so if B is ever false the compiler would throw an error saying that enable_if is not well defined.
// Forward declare enable_if
template<bool B, class T = void>
struct enable_if {};
// Only define the case where B is true
template<class T>
struct enable_if<true, T> { typedef T type; };
template <bool B, typename T>
using enable_if_t = typename enable_if<B, T>::type;
Attempting to construct this enable_if with B being false anywhere else in the program would cause the compiler to crash. But using it in the
template of a function signature allows SFINAE to deduce which signature we
would like to use.
// foo only works with floating point types
template <typename T, enable_if_t<std::is_floating_point<T>::value>>* = nullptr>
T foo(T x) {
return x;
}
// foo only works with integer types
template <typename K, enable_if_t<std::is_intergral<K>::value>>* = nullptr>
K foo(K x) {
return x;
}
// Calls the first signature
double x_dbl = 1.0;
double y_dbl = foo(x_dbl);
// Calls the second signature
int x = 1;
int y = foo(x);
The second template argument is referred to as a [non-type template
parameter](https://en.cppreference.com/w/cpp/language template_parameters#Non-type_template_parameter) and has a default value of void.
When the templated signature has the correct type the enable_if_t produces a void type which is then made into a pointer and assigned a default value of nullptr.
When the templated signature does not have the correct type, the compiler utilizes Substitution Failure Is Not An Error
(SFNIAE), to remove the offending signature from the list of possible matches while continuing to search for the correct signature.
There was a problem hiding this comment.
That looks good. I'll put that into the doc.
Co-authored-by: Steve Bronder <[email protected]>
Co-authored-by: Steve Bronder <[email protected]>
syclik
force-pushed
the
feature/simplify-meta
branch
from
96dc4e9 to
dd75ad7
Compare
|
@SteveBronder, I updated the last block of text! There were a couple of very minor edits made:
It really clarifies what's happening. Thanks!!!
My slight (very slight) preference is for it to go into develop. Reasons:
With all that said, I'm really not fussed if we put it on 5.0. |
|
I'm cool with merging to develop on this. Agree that those requires are not really part of the Stan Math API in the strict sense |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Summary
The mixed use of macros to generate a full set of template metaprograms is harder to read than having the template metaprograms in the code.
This PR expands the macros to the minimal set of macros required by the Math and Stan libraries.
Detail
There are 5 macros. I'll link to the latest released version (so the links are permanent), but they can be found on current develop. They are all inside stan/math/prim/meta/require_helpers.hpp:
All these macros, when used for every type, expands into a lot of definitions. Practically we don't use most of them.
Tests
None added.
Side Effects
None.
Release notes
Removes 5 macros and expands directly into code: STAN_ADD_REQUIRE_UNARY, STAN_ADD_REQUIRE_UNARY_INNER, STAN_ADD_REQUIRE_BINARY, STAN_ADD_REQUIRE_BINARY_INNER, STAN_ADD_REQUIRE_CONTAINER.
Checklist
Copyright holder: Daniel Lee
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested