Flatten scal / arr / mat

Summary:

Flatten the directory structure by getting rid of the scal, arr, mat folders. The goal is to make the Math library easier to navigate for developers and users.

Description:

Our current directory structure has looks like:

stan/math/
then {prim, rev, fwd, mix} indicating that we're dealing with no-autodiff, reverse mode, forward mode, or mixed mode
then {scal, arr, mat} indicated that we're dealing with scalar, array, or matrix, all from the point of view of the Stan language. Scalar is just double or the autodiff variable, array includes std::vector, and matrix includes Eigen.
then err for error checking, fun for functions, meta for template meta programs, functor for dealing with functors, prob for probability distributions
then finally functions are in separate headers.

We'd like to get rid of the {scal, arr, mat} level. We had originally done this to support the use-case of a client of the Math library wanting to compile with only scalars, only scalars + the standard template library (<vector> for std::vector), or compile with all of that and Eigen. I swear we had users request this back when we had the stan-users list on google groups, but I could never find that thread again. The motivation was compile times and minimal includes. (This might have been back in the days when compiling under Windows was limited due to lack of a decent compiler; those days are over.) This has caused a fair amount of technical overhead keeping these things separate. It would really simplify our development if we can assume we have access to std::vector and Eigen::Matrix in every function. Right now, we include the template metaprograms in the very delicate, correct order to instantiate things the right way. So... the order of instantiation is different in stan/math/prim/scal.hpp vs stan/math/prim/arr.hpp vs stan/math/prim/mat.hpp. What we want to do is to instantiate the template metaprograms in the order that the stan/math/*/mat.hpp instantiates them in.

The nice thing is that there is a correct way to instantiate the template metaprograms and we know what that is. We need to trace through those dependencies.

What the top-level includes will look like after we're done

1. stan/math/prim.hpp
2. stan/math/rev.hpp
3. stan/math/fwd.hpp
4. stan/math/mix.hpp

The current top-level includes

1. stan/math/prim/arr.hpp
2. stan/math/prim/mat.hpp
3. stan/math/prim/scal.hpp
4. stan/math/rev/arr.hpp
5. stan/math/rev/mat.hpp
6. stan/math/rev/scal.hpp
7. stan/math/fwd/arr.hpp
8. stan/math/fwd/mat.hpp
9. stan/math/fwd/scal.hpp
10. stan/math/mix/arr.hpp
11. stan/math/mix/mat.hpp
12. stan/math/mix/scal.hpp

Additional Information:

There are a few things we want to check:

compile times don't go up for standard models
everything still compiles

It's going to be a little tricky to do this. We should come up with some end-to-end tests for this to make sure it works on top of the lower-level unit tests that should continue to work.

Some unit tests will fail for the template metaprograms. The ones that will fail are for explicit checks that the base template metaprogram behaves in a certain way and we have specializations for std::vector or Eigen::Matrix. It's only once we include the specializations that the behavior will change.

The fix is to merge a bunch of files together. That's oversimplifying it, but that's essentially what we have to do. We just have to make sure things are merged in the right order. If done in that way, we'll maintain the correct behavior. I'll put together some examples below.

I believe we can script this so we don't have to do much by hand. In fact, I'm sure we can get a crude version working this way just by copying and pasting files in the right order.

There are going to be a lot of different ways we can do this. I'd like us to explore ways that keep the code-base working. Maybe even breaking this down into smaller chunks that we can merge as we go along. For example, one thing we may want to do is to merge all the template metaprograms into mat first and leave everything else alone.

We will need to modify make/test-math-dependencies. We will want to keep the rules for the autodiff types, but definitely not the ones for scal, arr, mat.

Current Version:

v2.17.0

for foward we also have core, is it preferred to just dump these in fwd or create a fwd/core subdirectory?

for foward we also have core, is it preferred to just dump these in fwd or create a fwd/core subdirectory?

so I believe you mean stan/math/fwd/core.hpp. I think I originally posted this before we had it. Either way, I think we can leave those. (Unless you mean something else.)

I posted this on discourse, but if you have one or more of these skills, please join the effort:

shell programming (we want to script this)
regex skills (sed / awk would do since we’re going to be replacing things in files)
being able to trace through precompiler dependency generation (see the -M flag in g++)
template metaprogramming and testing of them; we don’t have enough tests in place to do a refactor of this scale easily, so we’ll be writing a lot as we go
refactoring… the concept of it, understanding how to do it, just being careful
strong git skills; we’ll probably need to do some git bisecting as we do this

One place to start is to consolidate the template metaprograms. We can start at the top of stan/math/prim/mat.hpp

#include <stan/math/prim/arr/meta/get.hpp>
#include <stan/math/prim/arr/meta/index_type.hpp>
#include <stan/math/prim/arr/meta/is_vector.hpp>
#include <stan/math/prim/arr/meta/length.hpp>

#include <stan/math/prim/mat/meta/broadcast_array.hpp>
#include <stan/math/prim/mat/meta/get.hpp>
#include <stan/math/prim/mat/meta/index_type.hpp>
#include <stan/math/prim/mat/meta/is_constant_struct.hpp>
#include <stan/math/prim/mat/meta/is_vector.hpp>
#include <stan/math/prim/mat/meta/is_vector_like.hpp>
#include <stan/math/prim/mat/meta/length.hpp>
#include <stan/math/prim/mat/meta/length_mvt.hpp>
#include <stan/math/prim/mat/meta/operands_and_partials.hpp>
#include <stan/math/prim/mat/meta/scalar_type.hpp>
#include <stan/math/prim/mat/meta/seq_view.hpp>
#include <stan/math/prim/mat/meta/value_type.hpp>
#include <stan/math/prim/mat/meta/vector_seq_view.hpp>

If you look at the includes for get.hpp, you'll notice that the arr version needs to be included before the mat version. So when we consolidate the files, it'll look like:

#ifndef STAN_MATH_PRIM_ARR_META_GET_HPP
#define STAN_MATH_PRIM_ARR_META_GET_HPP

#include <cstdlib>
#include <vector>

namespace stan {

template <typename T>
inline T get(const std::vector<T>& x, size_t n) {
  return x[n];
}

}  // namespace stan
#endif

#ifndef STAN_MATH_PRIM_MAT_META_GET_HPP
#define STAN_MATH_PRIM_MAT_META_GET_HPP

#include <stan/math/prim/mat/fun/Eigen.hpp>

namespace stan {

template <typename T, int R, int C>
inline T get(const Eigen::Matrix<T, R, C>& m, size_t n) {
  return m(static_cast<int>(n));
}

}  // namespace stan
#endif

Cleaned up, of course.

Yep that’s what I mean. So I’ve been ‘cat’int the mat/arr/scal.hpp flies together, and using Linux sed to remove strings from the include that sets stuff back a directory. Is it worth a PR if tests aren’t passing, for the purpose of review? I tried doing just fwd but that depends on prim so I’m starting prim now

Yes, I think that makes sense. I think we're going to be going back and forth a lot on this one. We'll take a few steps forward, update a script, look at it carefully, figure out if it works, have to back it out, rinse and repeat.

It'll help to have GitHub's PR framework to compare.

Can you create a branch on this repo? It'll make it easier to coordinate.

created branch code-cleanup/issue-937-flatten which only contains the prim.hpp new top level include, if that's sufficient I can start making directories fun, functor, err in prim and start combining the files.

I'll try not to abruptly leave next time we're mid conversation

@syclik

@syclik you're probably offline because it's saturday, but I'll be online all day monday and we can get this rolling, if you're available.

EDIT: This is a bit too crude to be helpful. Hopefully my next post is more helpful.

In the hopes of getting my head around all the details, here's my first thought for a bash script.

folders=("scal" "arr" "mat")

for fldr in "${folders[@]}";
do
    for fullpath in stan/math/prim/"$fldr"/meta/*;
    do
        dirname="$(basename $(dirname $fullpath))"
        filename="$(basename $fullpath)"
        if [ "$filename" == "get.hpp" ];
        then
            if [ ! -d "stan/math/prim/$dirname" ];
            then
                mkdir "stan/math/prim/$dirname"
            fi
            echo "cat $fullpath >> stan/math/prim/$dirname/$filename"
            # cat "$fullpath" >> "stan/math/prim/$dirname/$filename"
        fi
    done
done

Some follow up thoughts.

Still missing a loop around prim, fwd, rev, and mix,
Not all of prim, fwd, rev, and mix have .hpp files two levels deep. For instance, core has .hpp files one level below fwd.
I've surely missed something(s), but I don't immediately see what.

What do you think?

Here's my next attempt to contribute something to this effort.

# run from within path/to/math
# ONLY prim
dir_dimensions=("scal" "arr" "mat")
dir_codebase=("./stan/math/prim" "./test/unit/math/prim")

for dir_dim in "${dir_dimensions[@]}";
do
    for dir_code in "${dir_codebase[@]}";
    do
        for dir_fn in "$dir_code/$dir_dim"/*;
        do
            dir_fn_name="$(basename $dir_fn)"

            # skip test/unit/math/prim/util.hpp
            # cause it doesn't follow my pattern
            if [ "$dir_fn_name" = "util.hpp" ];
            then
                # echo "skip $dir_fn_name"
                continue
            fi

            # Make directories, if needed
            if [ ! -d "$dir_code/$dir_fn_name" ];
            then
                # echo "mkdir $dir_code/$dir_fn_name"
                mkdir "$dir_code/$dir_fn_name"
            fi

            for filepath in "$dir_fn"/*;
            do
                # Copy files
                filename="$(basename $filepath)"
                # echo "cat $filepath >> $dir_code/$dir_fn_name/$filename"
                cat "$filepath" >> "$dir_code/$dir_fn_name/$filename"

                # Edit includes
                # loop again to catch where mat includes scal/arr, etc.

                # echo "sed -i.tmp s/\(\#include\ \<stan\/math\/prim\/\)$any_dim\(.hpp\>\)/\1\2/g $dir_code/$dir_fn_name/$filename"
                # echo "sed '-i.tmp' s/\(\#include\ \<stan\/math\/prim\/\)$any_dim\/\($dir_fn_name\/$any_filename\>\)/\1\2/g" "$dir_code/$dir_fn_name/$filename"

                for any_dim in "${dir_dimensions[@]}";
                do
                    # top-level
                    sed '-i.tmp' "s/\(\#include\ \<stan\/math\/prim\)\/$any_dim\(.hpp\>\)/\1\2/g" "$dir_code/$dir_fn_name/$filename";
                    sed '-i.tmp' "s/\(\#include\ \<test\/unit\/math\/prim\)\/$any_dim\(.hpp\>\)/\1\2/g" "$dir_code/$dir_fn_name/$filename";

                    # individual files
                    sed '-i.tmp' "s/\(\#include\ \<stan\/math\/prim\/\)$any_dim\/\(.*\>\)/\1\2/g" "$dir_code/$dir_fn_name/$filename";
                    sed '-i.tmp' "s/\(\#include\ \<test\/unit\/math\/prim\/\)$any_dim\/\(.*\>\)/\1\2/g" "$dir_code/$dir_fn_name/$filename";
                done
            done
        done
    done
done

# TODO
# Delete old files
# Delete sed's .tmp files
# Move includes to top of file
# Drop duplicate includes

I'll try to be around tomorrow to join the team effort.

I did follow the above discussion on and off but dont remember all the details so maybe this shares some of the ideas already discussed.

Below is for the /prim folder but the same goes for /fwd, /mix and /rev. We can leave /opencl out of this discussion for now, but that folder should also be revisited.

In each folder we have {err, fun, functor, meta} and the same principles can be applied for all. Example given for err.

PR 1: make a top-level header prim/err.hpp (For the meta subfolder this is already done) and replace all includes outside /prim/*/err/ with the top-level include. This will touch a lot of files but should make the next steps cleaner. Should also be straightforward to review, but lengthy. This PR should also include a check to prevent any subsequent PRs to break this work. This happened a lot for the meta.hpp before we added https://github.com/stan-dev/math/blob/develop/runChecks.py#L260

PR 2: Flatten the tests for prim/*/err in prim/err.

PR 3: Flatten all the files to the folder with the most functions (probably prim/err/scal) and then move them to prim/err/ and fix the top-level header. I dont see another way then just copy pasting stuff in manually and go from there. This way would preserve the most git history for the files, I think. I am very much open to suggestions here.

PR 1 is going to touch the most files but should not cause much issues for other developers. PR 2 will probably also not be problematic. PR 3 is going to be the most problematic in terms of additional work for other developers when they will merge the changes. Outside of generalizing signatures there arent many long-term projects happening right now. There is the kernel generator but that is limited to /opencl. So this might be a great time to do this.

Starting with #1476

Ok, I have 3 PRs lined up that would flatten prim/meta:

#1476 flattens the tests for prim/meta
#1479 cleans up the tests for redundancy
#1480 does the flattening of stan/math/prim/*meta

I think this is the easiest way of doing it in terms of reviewing. We could also do meta flatten prim/rev/fwd/mix together but I feel that will be more difficult to review.

Thanks! I'll take a look tonight / tomorrow.

…

On Sun, Dec 1, 2019 at 8:46 AM Rok Češnovar ***@***.***> wrote: Ok, I have 3 PRs lined up that would flatten prim/meta: #1476 <#1476> flattens the tests for prim/meta #1479 <#1479> cleans up the tests for redundancy #1480 <#1480> does the flattening of stan/math/prim/*meta I think this is the easiest way of doing it in terms of reviewing. We could also do meta flatten prim/rev/fwd/mix together but I feel that will be more difficult to review. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#937?email_source=notifications&email_token=AADH6F7WSJ7ANQQQ7AK63UTQWO5ZXA5CNFSM4FJQIYA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFRJVEI#issuecomment-560110225>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADH6F7OVISZTCE3KWR263LQWO5ZXANCNFSM4FJQIYAQ> .

Meta is now flattened and I think it went fairly well so if there are no objections I would proceed with flattening the /err folder (any suggestion by the reviewers are welcome off course).

The plan is to start by creating {prim, rev, mix, fwd}/err.hpp files and replace all includes of any /err header outside the err folder with the include of the respective master err.hpp header file. This is the same as we did for /meta.

After that we proceed with the same schedule of PRs as with the meta flatten (see this comment).

Go for it!

…

On Mon, Dec 16, 2019 at 9:27 AM Rok Češnovar ***@***.***> wrote: Meta is now flattened and I think it went fairly well so if there are no objections I would proceed with flattening the /err folder (any suggestion by the reviewers are welcome off course). The plan is to start by creating {prim, rev, mix, fwd}/err.hpp files and replace all includes of any /err header outside the err folder with the include of the respective master err.hpp header file. This is the same as we did for /meta. After that we proceed with the same schedule of PRs as with the meta flatten (see this comment <#937 (comment)>). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#937?email_source=notifications&email_token=AADH6FYAWBVER4DTMCVH6GLQY6F4BA5CNFSM4FJQIYA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG64DLQ#issuecomment-566084014>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADH6FZEHEOMS2RVTW2IGFTQY6F4BANCNFSM4FJQIYAQ> .

Yes, please. I now think we should keep going and also flatten prim/rev/fwd/mix with the goal of having one file per function. But that's for the future.

…

On Dec 16, 2019, at 9:27 AM, Rok Češnovar ***@***.***> wrote: Meta is now flattened and I think it went fairly well so if there are no objections I would proceed with flattening the /err folder (any suggestion by the reviewers are welcome off course). The plan is to start by creating {prim, rev, mix, fwd}/err.hpp files and replace all includes of any /err header outside the err folder with the include of the respective master err.hpp header file. This is the same as we did for /meta. After that we proceed with the same schedule of PRs as with the meta flatten (see this comment). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

@mcol

Once the tests pass we will have the meta, err and prob folders flattened. Thanks @mcol for all the thorough reviews of the recent PRs. I know they are painful to review. I also have the /functor flatten up on a branch. That one is really simple so it will be done in a single PR. It will require PRs on Stan/Cmdstan to fix include paths but those should be quick (see stan-dev/stan#2872).

So the first question is do we aim for having scal/arr/mat flattened entirely for the next release? Or should I hold off on flattening /fun after the release? If we should go through with it for the upcoming the second question is can we flatten /fun in the feature freeze period? I figure that would be the best time to do it as flattening /fun is going to cause a bunch of conflicts for developers trying to get their PRs through before the freeze. We could probably flatten fwd/fun and mix/fun without causing too much trouble for other open PRs, but prim/fun and rev/fun will be more problematic.

My proposal would be to try and flatten fwd and mix before the feature freeze and then flatten prim and rev right after the freeze (11th Jan).

The other question is the order of merge in /fun. If a file has arr, mat and scal implementations what would be the preferred order. I would say either scal, arr, mat or mat, arr, scal to have ascending or descending dimensions. But am open to any other suggestions.

I think scal should be first, as arr and mat often depend on scal implementation. Afaik arr and mat usually do not depend on each other so the order of these two should not matter.

So the first question is do we aim for having scal/arr/mat flattened entirely for the next release? Or should I hold off on flattening /fun after the release?

I don't think we need to worry about release schedules.

Navigation Menu

Repository navigation

Flatten scal / arr / mat #937

Summary:

Description:

What the top-level includes will look like after we're done

The current top-level includes

Additional Information:

Current Version:

55 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Flatten scal / arr / mat #937

Description

Summary:

Description:

What the top-level includes will look like after we're done

The current top-level includes

Additional Information:

Current Version:

Activity

drezap commented on Feb 22, 2019

syclik commented on Feb 23, 2019

syclik commented on Feb 23, 2019

syclik commented on Feb 23, 2019

drezap commented on Feb 23, 2019

syclik commented on Feb 23, 2019

drezap commented on Feb 23, 2019

drezap commented on Feb 23, 2019

roualdes commented on Feb 24, 2019

roualdes commented on Feb 25, 2019

55 remaining items

rok-cesnovar commented on Nov 30, 2019

rok-cesnovar commented on Nov 30, 2019

rok-cesnovar commented on Dec 1, 2019

syclik commented on Dec 3, 2019

rok-cesnovar commented on Dec 16, 2019

syclik commented on Dec 16, 2019

bob-carpenter commented on Dec 16, 2019

rok-cesnovar commented on Jan 2, 2020

t4c1 commented on Jan 2, 2020

bob-carpenter commented on Jan 2, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions