The Flattening Refactor


#1

Here’s an update of the flattening refactor, tagging everyone involved or interested:
@syclik @seantalts @roualdes

Looks like @roualdes’ script worked pretty well. We have all tests passing for the following prim directories: functor, meta, functor and vectorize.

I’m currently tying up some lose ends in prob, err and fun (it’s either some template deduction failures or the includes). After that, with a review, I think we can merge this into develop and start flattening rev, fwd and mix.

All of this is happening on code-cleanup/issue-937-flatten if anyone wants to follow along.


#2

now prim/prob is good


#3

yeah, everything is passing except for the test cov_matrix_transform_test.cpp, some templating issue. I’ll figure it out eventually.


#4

Great work, thanks.

Let me know if you have some follow up thoughts on how to improve this script for rev, fwd, and mix.

I’m just going to record here some other ideas.

At the end of this effort, go through all the files and remove the excess new lines that my script creates. Single spaced, to me, seems better than what my script currently leaves behind.

I noticed that the following tests are empty save #includes. Are these files here to remind us that these functions need tests?

test/unit/math/prim/mat/fun/typedefs_test.cpp
test/unit/math/prim/mat/err/constraint_tolerances.cpp
test/unit/math/prim/mat/err/validate_non_negative_index_test.cpp
test/unit/math/prim/mat/fun/LDLT_factor_test.cpp
test/unit/math/prim/mat/fun/columns_dot_product_test.cpp
test/unit/math/prim/mat/fun/dot_product_test.cpp
test/unit/math/prim/mat/fun/log_determinant_test.cpp
test/unit/math/prim/mat/fun/mdivide_left_tri_low_test.cpp
test/unit/math/prim/mat/fun/mdivide_right_tri_low_test.cpp
test/unit/math/prim/mat/fun/rows_dot_product_test.cpp


#5

probably! good catch, we can add this to the list of things to in “paying off math debt”.

I’ll give it some thought after we can successfully merge prim.

couldn’t agree more.


#6

Hey @roualdes -

So with the last commit I just pushed to code-cleanup/issue-937-flatten, everything in the refactored prim should compile. I think it’s safe for you to (if you don’t mind) go ahead and remove the extraneous blank lines at the top of the files (or between the includes and the #ifndef statements).

If it’s cool, please make sure the tests compile with your OS (as I’m not running mac OS), and then if that’s all good, go ahead and remove the extra lines.

I’m just getting one test error (it compiles fine) in prim/fun, that is, in test/unit/math/prim/fun/digamma_test.cpp. Not sure, nothing has changed really. I’ll take a look in the near future.

After both of these items are taken care of, I think we can ping DJ Syclik for a review, and we can get the prim refactor merged.


#7

Do you have an example file where blank lines happen

? I know there’s plenty of consecutive blank lines, that I plan on removing, but I can’t immediately see why my flatten.sh script would introduce blank lines between directives.

Either way, I’ll get started this weekend.


#8

Sorry for the delay.

There is extra white space between #include statements and namespace stan { statements.

Also, in for example, stan/math/prim/fun/common_type.hpp, we have two namespace stan declarations, with many white lines in between. If it’s easy to handle this via bash scripting, I would go ahead and fix it, but if not, I think we’re fine as long as all tests compile and pass.

But pretty much every file has extra blank lines that will cause lint errors and should be removed.

Andre


#9

You might want to check that the formatter doesn’t just take care of this


#10

@drezap and @roualdes, current status? And what’s the rough plan? (How many PRs to get to the finish?)


#11

No worries, the way my weekend went I’m bound to be the slowest link here.

Right, I did not pay attention to multiple namespace declarations because tests will still pass despite this. I’m hoping the same strategy for cleaning up multiple #ifndefs should work here. I’ll try to remove these along with double blank lines.

I’m about to start tidying up prim today. Then we’ll submit a PR for a backwards compatible, flattened prim.

It’s my understanding that we’ll have one PR for each of prim, rev, fwd, and mix. So 4 total.

Thanks, I’ll look into that.


#12

@roualdes regarding @sakrejda’s comment, I think the autoformatting will actually handle on the formatting issues you were going to address.

Let me just figure out why this one test isn’t passing, open a PR, and I think the autoformatter will handle it all.

I’ll ping you and let you know if we actually need to format this stuff manually.

Shooting to have this one test case figured out today.


#13

@drezap, from what I can see, the auto-formatter handles the blank spaces, but not the multiple namespaces.

I’m fairly close to a solution, for files that have commented the closing braces of their namespaces, e.g.

namespace stan {
namespace math {
// ...
} // namespace math
} // namespace stan

This could be a cleanup issue, if we want.


#14

As long as the code compiles and tests pass, I don’t see a need to add it to the current flattening, as it’s purely aesthetic, and serves no functional purpose. Unless @syclik really thinks this is an issue, then we can set a separate issue about it. It’s more important to get the flattening working first.


#15

This is super cool! I’m so looking forward to this.

Wherever possible, tests can be moved to use higher-level includes like rev/mat.hpp, etc.