operands_and_partials refactor

[seantalts]

Submission Checklist

• Run unit tests: ./runTests.py test/unit
• Run cpplint: make cpplint
• Declare copyright holder and open-source license: see below

Summary:

Replace the existing OperandsAndPartials with a new one that attempts to be a little less confusing (YOU be the judge!). It also adds multivariate support.

Documentation:

Reviewer Suggestions:

I had a few questions and wasn't sure how to do some things throughout but tried to add comments in the code with a TODO or something similar to catch these situations. That said, the code seems to work on all the tests that I can run in a short amount of time (will use Jenkins to run the full distribution tests, etc).

I also haven't added any doc yet; I plan to doc the main operands_and_partials class while the tests are running on Jenkins (but probably not the stuff I've added in the detail namespace).

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Dual Space LLC

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

• Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
• Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

[seantalts]

Let me fix the tests before anyone wastes time on this!

[syclik]

@seantalts, I'll take a look at this over this week.

Quick question: can we come up with a better name?

[syclik]

oh yeah... you think it's time for a review?

[seantalts]

So I sat with Bob today went over the design for a while and got some changes to make. I'll hopefully finish those up tomorrow - I'll post back here and tag you when I'm done? Or it might make sense to just have @bob-carpenter do the review since he's fairly steeped in the new design at this point.

[syclik]

Great! Please ping me anyway. I'd like to see the changes and having an extra set of eyes never hurt on something like this.

…

On Mon, May 8, 2017 at 10:58 PM, seantalts ***@***.***> wrote: So I sat with Bob today went over the design for a while and got some changes to make. I'll hopefully finish those up tomorrow - I'll post back here and tag you when I'm done? Or it might make sense to just have @bob-carpenter <https://github.com/bob-carpenter> do the review since he's fairly steeped in the new design at this point. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#547 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_Fw0ZPFkpFe8v1AZJaDvthkyXoeEbks5r39ZFgaJpZM4NQAv4> .

[seantalts]

Will do!

[seantalts]

Re: #547 (comment) I think you're right and it likely makes sense to expose the partials type on the edge classes. I'll give that a shot and work on hiding their implementation details.

[seantalts]

So I can do this kind of thing and use SFINAE to provide these methods only when appropriate (due to univariate o&p having partials_t and multivariate having partials_vec_t):

      typedef detail::ops_partials_edge<ViewElt, Op1> edge1_t;
      template <typename T = edge1_t>
      typename T::partials_vec_t& partials1_vec() { return edge1_.partials_vec_; }

Thoughts on this? It also uses a C++11 feature, default function template parameters. Some alternatives:

1. Break out multivariate from univariate
2. leave things as is
3. declare that the edges and the base operands_and_partials classes are friends, and declare those implementation details to be private.

I think 3 seems like the easiest for right now...

[bob-carpenter]

This is terrible for threaded commenting as I can't tell what #547 (comment) is in response to.

What is all this complicated template programming for?

[seantalts]

I was confused too at first but I think if you click the link it will bring the relevant comment to the top of your page.

In any case, the higher level goal is to hide dx(), dump_partials(), etc from end users. In the complicated templating post above, I was doing this by hiding the edges and just exposing a method to get at their contained partials. The template is used to make sure the partials1_vec() method only exists (using SFINAE) when there is an underlying partials_vec_ member on the edge1_ member, which happens when it's what I've been calling a "multivariate" edge (i.e. its operands are something like std::vector<Eigen::Matrix<...>>

[syclik]

Please feel free to ignore a bulk of the code-level comments. I was reviewing the pull request with this conversation from Discourse in mind. My memory is pretty hazy, but I recalled a request (in person) for a deep review at the API boundary, which is what I provided. It was a discussion between documenting and testing internal pieces vs just what's visible as a consumer of the class. Once I started looking at what the probability distribution functions use, it turns out there's a lot more complexity there. The internal classes are exposed directly and not testing those pieces directly makes it really tough to test from the outside.

@seantalts, as I mentioned in an earlier comment before, I'm happy with it going in as-is or possibly with a little work. I'm not expecting everything I mentioned to get fixed (ever). This does leave us in a better place than where we were. I guess the only change I'd want is for the thing I marked as a bug to be made safe.

If there's discrepancy between what I suggest and @bob-carpenter suggests, I'd say go with @bob-carpenter's suggestion.

To be helpful: @seantalts, what could I do to help? Should I put together a list of things that should be finished before merging? Should I add tests? Should I just sit back and let you finish it out?

[syclik]

@seantalts, although the code is unsafe, it can't be instantiated with the way our probability distributions currently work. I'm going to change the comment to requesting doc instead of fixing the code. There are still ways to construct it so it'll break, but I don't think we can do that with our current use cases.

[seantalts]

I appreciate the thorough review, sorry for the push back - just didn't want to spend a ton more time on it. I figured out a way to use friendship to limit exposure of those internal methods so we don't have to consider them as part of the public API.

I will figure out a solution for the BUG above if operands_ is size 0 and change the detail to internal. Are there any major classes of instantiation you think are missing that I could add quickly?

I also responded to the "var reuse" thing with more questions here, not sure if you saw it? I can try to do something about this as well if you think it's important.

[bob-carpenter]

For the var reuse thing, I don't think you need to worry about the foo(a, a) case and consolidating the a variables. We'll just take the hit for the extra overhead. It'd be hugely complicated to try to reduce this and probably of very limited use (not many distros where you repeat parameters, except the Dirichlet or some symmetric multinomials). I don't think you need to dream up possible instantiations here. If we have a sufficient set to implement our distributions, we can always add more specializations later for efficiency. I haven't been too worried in reviews about carving out a public API because we haven't done it anywhere else. I am glad you're starting to think about it as I do think it's important. When we're both back in the office, I'd like to talk about the general testing framework I have in mind that should allow us to blast through 4^10 possibilities. Whether we always want to do that is another question---if we could somehow set up tests with proofs by induction without having to repeat logic all the way up, that'd be great. But as is, we're trying to be defensive against downstream ad hoc optimizations.

[seantalts]

Sounds good, looking forward to it.

[bob-carpenter]

There's both a likely() and an unlikely(), so it depends how you want to arrange. With exceptions, it's usually unlikely() in an up-front test.

[seantalts]

(I think this is ready for review again)

[bob-carpenter]

Let's go over it tomorrow (Thursday) in person if that's OK. Then we should be able to resolve any last-minute things and get it merged.

[seantalts]

Okay, this is ready for another look. I think I addressed all of @bob-carpenter's comments on Thursday - some writing improvements and removing any inheritance from it, simplifying the conceptual burden at the expense of some additional code duplication. I'm pretty happy with it now.

[bob-carpenter]

Great. I can take a pass through this Monday.

[bob-carpenter]

Looks good. Some forward-looking comments, but nothing to change here.

[bob-carpenter]

If they're always zero size, why have them at all? It's not like they get get inherited by specializations or anything.

[seantalts]

They are needed to compile - even though the code will never run, the compiler needs to see that edge1_.partials_ exists and is the right type even for arithmetic primitives.

[bob-carpenter]

Don't worry about fixing this, but going forward, I'd much rather see typedefs of the form op_t rather than Op. Using Op makes it visually ambiguous with a template parameter (and if it were OP, with a constant).

Approval from @syclik to go ahead after my final review.