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.

1 Like
#2

now prim/prob is good

1 Like
#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.

1 Like
#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

1 Like
#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.

1 Like
#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.

1 Like
#16

@drezap, @roualdes: how’s it going? Mind posting up a summary? Is there some way I can provide support?

I still haven’t seen a plan for how to go from the current state to the end state. Do you have one and I’ve missed it?

#17

@syclik, thanks for checking in. We’re making progress, but have a number of compilation errors remaining.

We continue to bounce back and forth between me and @drezap. I add a little bit to the bash script we’ve been calling flatten; found as a gist here. Andre solves many of the compilation errors based off my last run of flatten, and then I add those changes to the script.

The latest bit we’ve focused on is adding Bob’s suggestion for the tests to use higher-level includes.

It’s my understanding that our plan is to continue developing flatten, flattening all directories at once, until we are satisfied with our automated solution. After this, we’ll ask for people to delay merging into develop, merge develop into our branch, run flatten, and ask for a review.

1 Like
#18

Thanks, @roualdes.

We have all tests in prim compiling and passing, and I’ll start working on rev, mix and fwd soon.

1 Like
#19

Mind getting into more detail here? I think it’d be much better to split this out into multiple steps, but I can see how it’s done at once.

Asking for everything to stop on the Math library is a hard ask. It’s not impossible, but it is a bit inconvenient, especially if the Math library has to be locked down for an unspecified amount of time while this is in review.

If you want chat to plan this out, just let me know. For example, I think it’d make a lot of sense if this was done in small stages like:

  1. Add the top-level meta folders: stan/math/{prim, rev, fwd}/meta/ with files that are ~5 lines: header guards and an include for the mat version of the file. Update source and test files that use stan/math/*/*/meta/ with the new file. (This leaves all the source intact and doesn’t require any synching across branches.) Maybe move the tests here or add another step to just move all the tests.
  2. Combine the meta headers into the one in stan/math/{prim, rev, fwd}/meta/. Remove the stan/math/{prim, rev, fwd}/*/meta/ folders. (This leaves most of the Math library intact and everyone else that’s not working on meta should be ok… the merge conflict here is easy to deal with, but most likely, there won’t be a merge conflict and git will handle it automatically.)

Then working through each of the folders separately.

I’m really, really weary of changing the whole code base at once. It’s too hard to review and too hard to verify that things don’t break.

Let me know what you think… I’m not trying to say that’s the way you need to do it or that it’s the only way. That’s just one way to do it that’s minimally disruptive to the Math library.

#20

We don’t have to stop everything, I can just go clean up the stuff in the middle that wasn’t refactored with this PR. Once the flattening gets reviewed, we can go ahead and ask contributors to use the new structure. Not many people will be affected this way.

It’s just difficult to do one directory at a time, we’d be modifying other directories anyway to get tests to pass. We can look into it again