Breakup require_generics.hpp into multiple files

[SteveBronder]

Description

After looking at require_generics.hpp it's too big and confusing imo. What I'd like to do is take all the stuff in there, break it up so that require_generics.hpp is just the definitions for the different base requires_*, then move the specializations over to each folder the is_* typedef is associated with.

Example

is_var.hpp would look like

//...
#include <stan/math/prim/meta/require.hpp>
//...

/** \ingroup type_trait
 * Defines a static member named value which is defined to be false
 * as the primitive scalar types cannot be a stan::math::var type.
 */
template <typename T, typename = void>
struct is_var : std::false_type {};

template <typename T>
using require_var_t = require_t<is_var<std::decay_t<T>>>;

template <typename T>
using require_not_var_t = require_not_t<is_var<std::decay_t<T>>>;

template <typename... Types>
using require_all_var_t = require_all_t<is_var<std::decay_t<Types>>...>;

template <typename... Types>
using require_any_var_t = require_any_t<is_var<std::decay_t<Types>>...>;

template <typename... Types>
using require_all_not_var_t = require_all_not_t<is_var<std::decay_t<Types>>...>;

template <typename... Types>
using require_any_not_var_t = require_any_not_t<is_var<std::decay_t<Types>>...>;

Current Version:

v3.1.0

[t4c1]

You can also make code shorter and easier to add new requires by using some macros. You can check what I did in kernel_generator/binary_operation.hpp. I would suggest adding a macro that gets condition, such as is_var and generates whole set of requires (require_var_t, require_not_var_t, require_all_var_t, ...) and one for containers (is_eigen -> require_eigen_t, require_eigen_vt ...).

[SteveBronder]

Agree!

[t4c1]

While you are at this, can you also add some requires for differentiating between sparse and dense matrices?

[SteveBronder]

@t4c1 I started working on this, can you take a peek at the diff here? It doesn't need a full review, just want a spot check that this seems like the right direction

https://github.com/stan-dev/math/compare/cleanup/require_base

Essentially it adds macros

STAN_ADD_REQUIRE_UNARY: requires that operate on individual types
STAN_ADD_REQUIRE_UNARY_SCALAR: requires that operate on individual type's scalar type
STAN_ADD_REQUIRE_UNARY_VALUE: requires that operate on individual type's value type
STAN_ADD_REQUIRE_BINARY: requires that compares two types
STAN_ADD_REQUIRE_BINARY*_SCALAR: requires that compare two types's scalar type
STAN_ADD_REQUIRE_BINARY*_VALUE : requires that compare two types's value type

It also moves around the docs so the site should be a bit easier to browse. If you can run make doxygen and lmk if anything should be added there that would be rad

EDIT*: Had UNARY where I should have had BINARY

[t4c1]

That looks great! Although I have a few questions:
Where do we need STAN_ADD_REQUIRE_BINARY - it seems unused?
This is missing the requires with check both on container and scalar/value. Do you plan to remove them or are they just not yet in?
STAN_ADD_REQUIRE_UNARY_SCALAR and STAN_ADD_REQUIRE_UNARY_VALUE seem to be mostly used together. If they always are, you can replace them with single macro.
Macros seem to be mostly callec with M(X, is_X). If that is always so you can remove second parameter.

[bob-carpenter]

Why macros and not just more types? And with these macros, what's use going to look like? I worry about maintainability when there are functions X being defined where a search for X in the code fails.

[bob-carpenter]

With complex, we need to be more careful. We've already decided that complex numbers will be treated as scalars. The value type of a Matrix<complex<T>, ...> is complex<T> and the value type of complex<T> is T. The value type of fvar<fvar<T>> is fvar<T>, and the value type of fvar<T> is T. I think we need something that recursively drills down through all value types down to the underlying real type.

What's REQUIRE_BINARY for? Also, the last two, STAN_ADD_REQUIRE_UNARY_SCALAR and STAN_ADD_REQUIRE_UNARY_VALUE are repeated---was that a cut-and-paste error?

[SteveBronder]

Where do we need STAN_ADD_REQUIRE_BINARY - it seems unused?

It's for things like std::is_same and std::is_convertible ala
STAN_ADD_REQUIRE_BINARY(same, std::is_same);

This is missing the requires with check both on container and scalar/value. Do you plan to remove them or are they just not yet in?

Wanted to get your opinion on how I was going about this before I did all those

STAN_ADD_REQUIRE_UNARY_SCALAR and STAN_ADD_REQUIRE_UNARY_VALUE seem to be mostly used together. If they always are, you can replace them with single macro.
Macros seem to be mostly callec with M(X, is_X). If that is always so you can remove second parameter.

Yeah right now I'm defining both those macros in the scalar_type and value_type headers, but if I move some stuff around I can put them in the require_helpers header and make one macro

Why macros and not just more types?

Yes I too am not a huge fan of macros. But on this occasion if we don't use them we'll just have a ton of effectively duplicate code. Like say we wanted to add requires for all of the type traits in std. Without the macro we'll have a ton of essentially duplicate code but with the macro we'll just have

STAN_ADD_REQUIRE_UNARY(arithmetic, std::is_arithmetic);
STAN_ADD_REQUIRE_UNARY(array, std::is_array);

which feels a lot easier to maintain. We can make this safer by also adding a header to undefine the macros at the bottom of each file like

#include <stan/math/prim/meta/require_helpers.hpp>
/** \ingroup type_trait
 * Defines a static member function type which is defined to be false
 * as the primitive scalar types cannot be a stan::math::fvar type.
 */
template <typename T, typename = void>
struct is_fvar : std::false_type {};

/** \addtogroup require_stan_scalar
*  @{
*/
STAN_ADD_REQUIRE_UNARY(fvar, is_fvar);
/** @}*/
#include <stan/math/prim/meta/undef_require_helpers.hpp>

Then they only exist when processing individual headers which feels safer.

And with these macros, what's use going to look like? I worry about maintainability when there are functions X being defined where a search for X in the code fails.

Use will be exactly the same, this is a cleanup that will leave everything fully backwards compatible. The macros just generates the C++ requires aliases. The main thing to me is making sure these are doc'd well, Bob if you have time to check out that branch above and run make doxygen to check out the site would appreciate your opinion on how these should be broken up.

https://github.com/stan-dev/math/compare/cleanup/require_base#diff-18d0716c1223927ad79f050f1edb33adR58

For doc'ing these, I wouldn't hate having a submodule structure like the below on the lhs of the site

requires
    - base type requires
        - Scalar requires
            - Arithmetic
            - Var
            - ...
        - Container Requires
            - Eigen
            - Standard Vector
            - ...

The docs I have right now have some grouping but I think the 3rd level to break them up may be a good idea as well

What's REQUIRE_BINARY for?

See above, it's for std::is_same and such that need to compare two types

Also, the last two, STAN_ADD_REQUIRE_UNARY_SCALAR and STAN_ADD_REQUIRE_UNARY_VALUE are repeated---was that a cut-and-paste error?

Yep fixed it!

[bob-carpenter]

Thanks---that's very clear now.  It's cool the expansions show up in the doc.

I'd like to stress again that you need to plan for complex in all this, so I think scalar is one level too low to stop---we need to get down to the base real value type.

[SteveBronder]

I'd like to stress again that you need to plan for complex in all this, so I think scalar is one level too low to stop---we need to get down to the base real value type.

Yes def agree! Should there be a require_*_bt for checking the base type? And for the docs should we have it like

requires
    - base type requires
        - Scalar requires
            - Real Types
                - Arithmetic
                - Var
            - Complex Types // idk what else to call this level
                - Complex

[SteveBronder]

Also the only way I can think of doing this nicely without a macro would be to break backwards compatibility with something like

// Either T1 or T2 are an eigen type with arithmetic value types
require_t<T1, T2, value_type, require_any, is_eigen, std::is_arithmetic>

vs the alternative alias specialization now of

require_any_eigen_vt<std::is_arithmetic, T1, T2>

[SteveBronder]

Alright so with what I am going to admit is insane macro and doxygen voodoo I got doxygen to auto generate docs for each require and put them in categories. I'll fix some things up and try to put out a PR tmrw

[t4c1]

If we wanted to redesign the requires, we could also use nested structs and a generic negation to do something like:

requre<list of conditions>::scalar/value/base<optional list of conditions on scalar/value/base type>::check_t/check_any_t<list of types>

So that would give us both simple and generic interface. Some examples:

require<is_var>::check_t<T1,T2>
require<is_same>::check_t<T1, T2, T3>
require<is_eigen>::value<is_fvar>::check_t<T>
require<is_std_vector>::scalar<not<is_var>>::check_any_t<T1, T2>
require<>::scalar<std::is_arithmetic>::check_t<T>

I think this covers most if not all of our use cases. Also it limits code duplication and does not require macros.

[SteveBronder]

hmm, I do like that. @bob-carpenter how do you feel about the above? I have the macro stuff pretty much ready to rock at this point. The above would break backwards compatibility but it is nice!

[bob-carpenter]

What's breaking backward compatibility? I think the macros are OK for this purpose. It's the way Boost and Eigen do a lot of this. Was there something specific you wanted comments on? - Bob

…

On Fri, Mar 20, 2020 at 11:58 AM Steve Bronder ***@***.***> wrote: hmm, I do like that. @bob-carpenter <https://github.com/bob-carpenter> how do you feel about the above? I have the macro stuff pretty much ready to rock at this point. The above would break backwards compatibility but it is nice! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ2D73ISPUTQGJRDMLNZMLRIOHEFANCNFSM4K2BXQZA> .

[SteveBronder]

I was looking at @t4c1 's comment here

#1739 (comment)

But yeah I'm fine with the macros I should have it up today