Untangles all of our includes!

[syclik]

Summary:

This pull request untangles all of our includes.

For prim vs rev vs fwd:

• prim is standalone and does not include rev or fwd
• rev includes prim, but not fwd
• fwd includes prim, but not rev
• mix includes prim, rev, and fwd

For scal vs arr vs mat:

• scal is standalone
• arr includes scal, but not mat
• mat includes arr and scal

Most of this was just shifting code around. There were two major pieces that I had to rewrite in order to get this to work:

• VectorView
• OperandsAndPartials.

All tests still pass. This might reduce compile times a bit.

Intended Effect:

Detangle all our headers! Makes the math library easier to use. We can now maintain this going forward.

How to Verify:

Run tests. See VectorView and OperandsAndPartials to see if that makes sense. I added some more doc for those.

Side Effects:

Compile times might be slightly shorter!

Documentation:

Included.

Reviewer Suggestions:

Anyone.

P.S. Sorry for the really large number of files.

[syclik]

@bob-carpenter, I need to talk to you about how I allocated memory for the fvar specialization of OperandsAndPartials. It's in this file:
https://github.com/stan-dev/math/blob/feature/issue-246-tangled-includes/stan/math/fwd/scal/meta/OperandsAndPartials.hpp

I'll leave a note on the particular line so you can see what I'm talking about.

[syclik]

@bob-carpenter, this used to look like this:

all_partials(static_cast<T_partials_return*>
                       (vari::operator new
                        (sizeof(T_partials_return) * n_partials)))

Should I have left it alone? I was trying to trace back to a way to allocate on the right managed stack without having to touch anything in reverse mode (stan::vari::operator new is in reverse mode).

[bob-carpenter]

Yes, that's bad news. For every new (that is not from vari), you need to manage the memory and make sure its deleted. But I don't think that's what you want to do here.

Instead, how about just declaring (note the final _ because its a member variable):

  std::vector<T_partials_return> all_partials_;

and then in the ctor, all you'd need is this:

          all_partials_(n_partials);

That leaves the allocation up to std::vector, which will automatically manage it properly (it's basically the RAII pattern, which you could use to fix your code by managing the deallocation, but you might as well leave this to C++).

Now if you want to write a mixed case, that could do the allocation back on vari, which should be more efficient.

[bob-carpenter]

Ack, as I was closing that file I saw that you did the delete. That should be OK the way you have it. It might be more efficient than using a std::vector because it won't autofill with a dummy value (so the std::vector version should probably use the ctor (through the initializer):

   all_partials_(n_partials, T_partials_return(std::numeric_limits<double>::quiet_NaN()))

At least that way, there's only one erroneous debug.

So it really depends how safe you feel managing your own memory!

[syclik]

Sorry, I should have made it clear that I deleted the memory in the destructor.

I thought we would have been able to add things to another stack that we controlled without having to go through the reverse chainable, but I'm not realizing that we didn't expose anything like that.

We'll talk tomorrow about it, but if you think it's ok, there shouldn't be anything else on this pull request that's questionable. Just a lot of rearranging the deck chairs.

[bob-carpenter]

We could create another memory arena, but you'd have
to manage it through fwd. The one we have now is
specific to rev.

• Bob

On Mar 10, 2016, at 3:33 PM, Daniel Lee notifications@github.com wrote:

Sorry, I should have made it clear that I deleted the memory in the destructor.

I thought we would have been able to add things to another stack that we controlled without having to go through the reverse chainable, but I'm not realizing that we didn't expose anything like that.

We'll talk tomorrow about it, but if you think it's ok, there shouldn't be anything else on this pull request that's questionable. Just a lot of rearranging the deck chairs.

—
Reply to this email directly or view it on GitHub.

[syclik]

That's what I assumed. I think the old code was error prone if it was allocating on the rev memory arena in forward mode if we never recover memory.

Of course, we've never used forward mode in any of the production code, so it's a moot point.

On Mar 10, 2016, at 4:12 PM, Bob Carpenter notifications@github.com wrote:

We could create another memory arena, but you'd have
to manage it through fwd. The one we have now is
specific to rev.

• Bob

On Mar 10, 2016, at 3:33 PM, Daniel Lee notifications@github.com wrote:

Sorry, I should have made it clear that I deleted the memory in the destructor.

I thought we would have been able to add things to another stack that we controlled without having to go through the reverse chainable, but I'm not realizing that we didn't expose anything like that.

We'll talk tomorrow about it, but if you think it's ok, there shouldn't be anything else on this pull request that's questionable. Just a lot of rearranging the deck chairs.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[syclik]

Jenkins, retest this please.

[syclik]

I just took another sweep of this changes and I think it's accomplished what I was shooting for. Could someone else take a look and not take my word for it? I've reworked code for VectorView and OperandsAndPartials. The rest of the changes in the source directory are single header files being split into multiple ones to maintain the right dependencies.

[bob-carpenter]

I checked everything out and looked at it and grepped to make sure there weren't any cross-includes and that looks good.

It looks like VectorView will only work in scope, because everything is holding references. I think that's OK, because of our usage within density functions, but it should probably go into the doc (which is only in prim/scal/meta/).

Even in scope, it's potentially volatile because further use of a std::vector can change the underlying pointer and cause the memory the VectorView is holding to be invalid. So basically, once you have a view of a std::vector or Eigen::Matrix, the viewed object better not change, or the behavior on what we're holding is undefined. The alternative to holding a pointer to the raw memory would be to hold a reference to std::vector, which would then track the changes to the original vector (rather than having undefined behavior); it may be a bit less efficient, and may not be required. But I think this aspect also needs to be documented.

It also doesn't look like you could do VectorView<double>(4.0), because there's no reference with just a numeric literal. But then I think we only invoke it inside functions where the views are of arguments which are passed in as variables and only held as long as needed. It'd be straightforward (and more efficient, I think) to make the VectorView<double> instances hold a double rather than a reference to a double, but given that these views are mutable, it didn't seem like that would work. It seems like you really do need a reference or pointer if you want to implement non-const returns.

We already went through the OperandsAndPartials so that OK (at least by me).

I made the doc changes and pushed so if it still passes it's ready to go as far as I'm concerned.

[betanalpha]

I’ve always thought of the VectorViews as static, at least in how
we’ve used them in the density/CDF implementations. Perhaps
it would make this more explicit if we made the return a const and
renamed it to VectorConstView or something?

On Mar 16, 2016, at 2:53 AM, Bob Carpenter notifications@github.com wrote:

I checked everything out and looked at it and grepped to make sure there weren't any cross-includes and that looks good.

It looks like VectorView will only work in scope, because everything is holding references. I think that's OK, because of our usage within density functions, but it should probably go into the doc (which is only in prim/scal/meta/).

Even in scope, it's potentially volatile because further use of a std::vector can change the underlying pointer and cause the memory the VectorView is holding to be invalid. So basically, once you have a view of a std::vector or Eigen::Matrix, the viewed object better not change, or the behavior on what we're holding is undefined. The alternative to holding a pointer to the raw memory would be to hold a reference to std::vector, which would then track the changes to the original vector (rather than having undefined behavior); it may be a bit less efficient, and may not be required. But I think this aspect also needs to be documented.

It also doesn't look like you could do VectorView(4.0), because there's no reference with just a numeric literal. But then I think we only invoke it inside functions where the views are of arguments which are passed in as variables and only held as long as needed. It'd be straightforward (and more efficient, I think) to make the VectorView instances hold a double rather than a reference to a double, but given that these views are mutable, it didn't seem like that would work. It seems like you really do need a reference or pointer if you want to implement non-const returns.

We already went through the OperandsAndPartials so that OK (at least by me).

I made the doc changes and pushed so if it still passes it's ready to go as far as I'm concerned.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

[syclik]

VectorView has const correctness (it had a template specialization for it before this pull request).

But, it's functionality was extended at some point for use in OperandsAndPartials. (It could have been me) if we were to redesign it, which I would support, I would prefer simpler classes rather than a generic template implementation that covers many separate use cases.

On Mar 16, 2016, at 5:30 AM, Michael Betancourt notifications@github.com wrote:

I’ve always thought of the VectorViews as static, at least in how
we’ve used them in the density/CDF implementations. Perhaps
it would make this more explicit if we made the return a const and
renamed it to VectorConstView or something?

On Mar 16, 2016, at 2:53 AM, Bob Carpenter notifications@github.com wrote:

I checked everything out and looked at it and grepped to make sure there weren't any cross-includes and that looks good.

It looks like VectorView will only work in scope, because everything is holding references. I think that's OK, because of our usage within density functions, but it should probably go into the doc (which is only in prim/scal/meta/).

Even in scope, it's potentially volatile because further use of a std::vector can change the underlying pointer and cause the memory the VectorView is holding to be invalid. So basically, once you have a view of a std::vector or Eigen::Matrix, the viewed object better not change, or the behavior on what we're holding is undefined. The alternative to holding a pointer to the raw memory would be to hold a reference to std::vector, which would then track the changes to the original vector (rather than having undefined behavior); it may be a bit less efficient, and may not be required. But I think this aspect also needs to be documented.

It also doesn't look like you could do VectorView(4.0), because there's no reference with just a numeric literal. But then I think we only invoke it inside functions where the views are of arguments which are passed in as variables and only held as long as needed. It'd be straightforward (and more efficient, I think) to make the VectorView instances hold a double rather than a reference to a double, but given that these views are mutable, it didn't seem like that would work. It seems like you really do need a reference or pointer if you want to implement non-const returns.

We already went through the OperandsAndPartials so that OK (at least by me).

I made the doc changes and pushed so if it still passes it's ready to go as far as I'm concerned.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

[syclik]

Thanks for the updated doc, Bob.