Feature/0464 refactor seqview #486

seantalts · 2017-02-09T17:42:30Z

Submisison Checklist

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

Summary:

Refactor most common use case of VectorView (taking either a primitive or a sequence of primitives and treating it seamlessly as a sequence of primitives) into scalar_seq_view, which is much simpler.

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/)

The result of a day's pair programming with Bob. Next commit will replace usage.

Eventually the goal is to rewrite and replace all uses of VectorView with some form of seq_view.

bob-carpenter · 2017-02-09T19:22:44Z

Is that [WIP] supposed to mean this is work in progress? That makes sense if you are just using the test harness. But you'll need to let someone know when you want it reviewed.

seantalts · 2017-02-09T19:27:01Z

Yeah, that was my intention; think someone told me to use [WIP] at one point.

bob-carpenter · 2017-02-09T19:40:25Z

OK --- we can change that to a label at some point if we get too many of these and need to sort. Let's hope it doesn't come to that. Daniel's had some WIPs for a whole as issues. Usually we kill them after a while pending completion so our pull requests aren't cluttered. - Bob

…

On Feb 9, 2017, at 2:27 PM, seantalts ***@***.***> wrote: Yeah, that was my intention; think someone told me to use [WIP] at one point. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

syclik · 2017-02-09T19:47:48Z

Yeah -- we shouldn't have too many of those. The last pull request went from what I thought was complete to realizing it wasn't; we can kill it and I can reopen later if that helps. On Thu, Feb 9, 2017 at 2:40 PM, Bob Carpenter <notifications@github.com> wrote:

…

OK --- we can change that to a label at some point if we get too many of these and need to sort. Let's hope it doesn't come to that. Daniel's had some WIPs for a whole as issues. Usually we kill them after a while pending completion so our pull requests aren't cluttered. - Bob > On Feb 9, 2017, at 2:27 PM, seantalts ***@***.***> wrote: > > Yeah, that was my intention; think someone told me to use [WIP] at one point. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub, or mute the thread. > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F-lJc58H_vEkV6LJdFGjh_y3jOB-ks5ra2upgaJpZM4L8cYn> .

seantalts · 2017-02-10T04:35:53Z

@bob-carpenter Okay, tests pass and I'm marking this as non-WIP. I think it might be a good place to start with the refactor, keeping in mind your advice to keep pull requests as small as possible. Bob, since you and I paired on the design together do you think we should get a 3rd party to review it? And then whoever wants to can double check that I did the substitutions correctly... Actually maybe I should have just put up the design first and then done the replacements in a 2nd PR? What do you guys think?

syclik

Looks good; just needs documentation.

syclik · 2017-02-10T16:18:56Z

stan/math/prim/scal/meta/scalar_seq_view.hpp

+
+namespace stan {
+
+  template <typename C, typename T = typename scalar_type<C>::type>


Add documentation for the class. See https://github.com/stan-dev/stan/wiki/How-to-Write-Doxygen-Doc-Comments

syclik · 2017-02-10T16:19:13Z

stan/math/prim/scal/meta/scalar_seq_view.hpp

+    const C& c_;
+  };
+
+  template <typename T>


Add doc describing what this specialization is.

syclik · 2017-02-10T16:19:51Z

stan/math/prim/mat/meta/scalar_seq_view.hpp

+#include <stan/math/prim/scal/meta/scalar_seq_view.hpp>
+
+namespace stan {
+  template <typename T>


Add doc about the template specialization.

syclik · 2017-02-10T16:27:18Z

test/unit/math/prim/mat/meta/scalar_seq_view_test.cpp

+}
+
+TEST(MetaTraits, ScalarSeqViewMatrix) {
+  expect_scalar_seq_view_values(Eigen::VectorXd(4));


if you're bored, you can look at scoped traces in Google Test to give context when this fails. No need to take action, but it is a useful feature of Google Test.

Ooh, sounds cool! Will check it out, thanks.

syclik · 2017-02-10T16:28:50Z

test/unit/math/prim/mat/meta/scalar_type_test.cpp

-  bool b3 = is_same<double, scalar_type<Matrix<double, Dynamic, Dynamic> >::type>::value;
-  EXPECT_TRUE(b3);
+  // TODO(Sean): This behavior doesn't seem to make sense prima facie
+  // Likely change this so that we would expect `double` here instead.


Just commenting; I think scalar_type just unwraps one layer. I think there's a recursive version kicking around. But either way, this is improperly named.

It appears to be recursive on std::vector but not Matrix 🌝

seantalts · 2017-02-10T17:37:59Z

stan/math/prim/scal/meta/scalar_seq_view.hpp

+     * @param i index
+     * @return the element at the specified position in the container
+     */
+    const T& operator[](int i) const {


What do you guys think about these operator[]s being inlined? I'm still getting a feel for where that's appropriate.

[skip-ci]

seantalts · 2017-02-10T17:53:12Z

jenkins, retest this please

bob-carpenter · 2017-02-10T18:03:25Z

We're not that worried about size. Yes, they should probably be inline. There's two aspects of inline---the compiler hint and the translation unit duplication feature. Member functions automatically get the latter. See this: http://stackoverflow.com/questions/26241023/different-implementations-of-inline-functions-in-different-translation-units

seantalts · 2017-02-10T19:43:04Z

Re: inline - I just looked into it a little more, and some stuff suggests that all member functions declared inside a class are implicitly inline and that marking it as inline does nothing? http://stackoverflow.com/questions/9370493/inline-function-members-inside-a-class

betanalpha · 2017-02-10T20:01:14Z

Correct, that’s what I was trying to yell in the background.

…

On Feb 10, 2017, at 2:43 PM, seantalts ***@***.***> wrote: Re: inline - I just looked into it a little more, and some stuff suggests that all member functions declared inside a class are implicitly inline and that marking it as inline does nothing? http://stackoverflow.com/questions/9370493/inline-function-members-inside-a-class <http://stackoverflow.com/questions/9370493/inline-function-members-inside-a-class> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdNlunAIfCphazpz8ie0-SnuAW6t7fjks5rbL3JgaJpZM4L8cYn>.

seantalts · 2017-02-10T20:02:27Z

Schwoops. Well, I'll leave them unmarked then.

bob-carpenter · 2017-02-10T21:19:23Z

Yes, member functions are automatically inline in the sense that you can use the definition in multiple translation units. But what I was trying to say is that I'm not sure if that implies they are also inline in the nudge the compiler into unfolding sense. - Bob

…

On Feb 10, 2017, at 2:43 PM, seantalts ***@***.***> wrote: Re: inline - I just looked into it a little more, and some stuff suggests that all member functions declared inside a class are implicitly inline and that marking it as inline does nothing? http://stackoverflow.com/questions/9370493/inline-function-members-inside-a-class — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

seantalts · 2017-02-12T16:18:09Z

Bob, gotcha. I think from what I was reading on that SO link that literally nothing changes in the compiler if you mark a member function inline, though it's hard to imagine that being perfectly consistent across all compilers...

Daniel, I added doc if you want to take a quick look whenever you have a chance and see if it looks good now? Thanks!

syclik · 2017-02-12T20:00:33Z

Looks good to me. Just approved.

seantalts added 2 commits February 8, 2017 18:43

Initial VectorView replacement - scalar_seq_view

bd90f6b

The result of a day's pair programming with Bob. Next commit will replace usage.

Replace VectorView with scalar_seq_view in prob dists

ff4dda5

Eventually the goal is to rewrite and replace all uses of VectorView with some form of seq_view.

seantalts force-pushed the feature/0464-refactor-seqview branch from f85a20a to ff4dda5 Compare February 9, 2017 19:11

seantalts changed the title ~~[WIP] Feature/0464 refactor seqview~~ Feature/0464 refactor seqview Feb 10, 2017

syclik requested changes Feb 10, 2017

View reviewed changes

seantalts commented Feb 10, 2017

View reviewed changes

Add doc to new classes and specializations.

8e8aabb

[skip-ci]

Break up tests

2de024b

seantalts mentioned this pull request Feb 11, 2017

refactor sequence views #464

Closed

syclik approved these changes Feb 12, 2017

View reviewed changes

syclik merged commit d544624 into develop Feb 12, 2017

syclik deleted the feature/0464-refactor-seqview branch February 12, 2017 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/0464 refactor seqview #486

Feature/0464 refactor seqview #486

seantalts commented Feb 9, 2017 •

edited

Loading

bob-carpenter commented Feb 9, 2017

seantalts commented Feb 9, 2017

bob-carpenter commented Feb 9, 2017 via email

syclik commented Feb 9, 2017 via email

seantalts commented Feb 10, 2017

syclik left a comment

syclik Feb 10, 2017

syclik Feb 10, 2017

syclik Feb 10, 2017

syclik Feb 10, 2017

seantalts Feb 10, 2017

syclik Feb 10, 2017

seantalts Feb 10, 2017

seantalts Feb 10, 2017

seantalts commented Feb 10, 2017

bob-carpenter commented Feb 10, 2017 via email

seantalts commented Feb 10, 2017

betanalpha commented Feb 10, 2017 via email

seantalts commented Feb 10, 2017

bob-carpenter commented Feb 10, 2017 via email

seantalts commented Feb 12, 2017 •

edited

Loading

syclik commented Feb 12, 2017


		namespace stan {

		template <typename C, typename T = typename scalar_type<C>::type>

Feature/0464 refactor seqview #486

Feature/0464 refactor seqview #486

Conversation

seantalts commented Feb 9, 2017 • edited Loading

Submisison Checklist

Summary:

Copyright and Licensing

bob-carpenter commented Feb 9, 2017

seantalts commented Feb 9, 2017

bob-carpenter commented Feb 9, 2017 via email

syclik commented Feb 9, 2017 via email

seantalts commented Feb 10, 2017

syclik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seantalts commented Feb 10, 2017

bob-carpenter commented Feb 10, 2017 via email

seantalts commented Feb 10, 2017

betanalpha commented Feb 10, 2017 via email

seantalts commented Feb 10, 2017

bob-carpenter commented Feb 10, 2017 via email

seantalts commented Feb 12, 2017 • edited Loading

syclik commented Feb 12, 2017

seantalts commented Feb 9, 2017 •

edited

Loading

seantalts commented Feb 12, 2017 •

edited

Loading