VectorViewMvt broadcasting logic question

Developers
1

Hey,

So a friend and I are pairing to writing tests for VectorViewMvt and we think we might have found a bug in the case where a vector of matrices is passed in, though it’s not obvious what the intended behavior would be in that case either.

So right now no code passes in a std::vector of matrices, but we wrote tests for it and when that happens, is_array is looking at the type of the Matrix in order to determine if it should be indexable (instead of the fact that it was constructed with a std::vector) and then uses that information to determine if we should be allowed to iterate through the std::vector or not. So that seems wrong? And then it’s not clear what the behavior should be; if we do have a std::vector of matrices right now ‘i’ will be allowed to index into it as normal, meaning it won’t be broadcast at all. I might think that 1) since this behavior doesn’t seem to be used(?) and 2) it doesn’t act in line with the other form of broadcasting this class does, maybe we should just remove it and always return the first matrix in the csae of a std::vector initializer or a single matrix initializer?

Thoughts?
-Sean

2

The behavior should be that there are three classes:

ScalarSeqView (now called: VectorView)
VectorSeqView
RowVectorSeqView
MatrixSeqView (now called: VectorViewMvt)

Each one should behave the same way and provide an expression template
that provides a sequence-like view. It should be constructable out of
either a T or a std::vector. For the former, every index returns the
value provided in the constructor. For the latter, indexing works as
normal.

So it sounds like you found a bug. I’m not sure why that class is
getting used if not to wrap matrices or std::vectors of matrices.

Let me give this some thought. We’ve gotten way better at writing
these things since the first one went in.

  • Bob
3

Basically, this is what we want, but it’s not well-formed C++ because
you can’t define a class twice:

template
struct SeqView {
T x_;

SeqView(const T& x) : x_(x), xs_(0) { }

T operator[](int n) const {
return x_;
}
};

template
struct SeqView {
vector xs_;

SeqView(const vector& xs) : xs_(xs) { }

T operator[](int n) const {
return xs_[n];
}
};

So all that fancy footwork is just to provide a way to define a single
class that effectively supports both of the above constructors and has
the appropriate operator[] behavior. It might be easily simplified and
hopefully it’ll be generalizable so we don’t need three different wrappers.

I think the one place things got confusing is that VectorView may right
now support matrix arguments, which I think was a mistake. I’m not sure
we use that anywhere, either.

  • Bob
4

Trying again with code formatting:

template <typename T>
struct SeqView {
  T x_;

  SeqView(const T& x) : x_(x), xs_(0) { }

  T operator[](int n) const {
    return x_;
  }
};

template <typename T>
struct SeqView {
  std::vector<T> xs_;

 SeqView(const std::vector<T>& xs) : xs_(xs) { }

  T operator[](int n) const {
    return xs_[n];
  }
};
5

Gotcha. So how do we actually translate that into C++? It seems like we could do one of two things:

  1. In the singular case, wrap it in a std::vector
  2. In the constructor that takes a std::vector , set a private bool is_array flag to true and use that instead of the template parameter (so now there’s an additional if check at runtime

And JTBC, in your code sample the semantics copy the std::vector input, right? The current version keeps a non-owning pointer to it which does seem dangerous so that seems like a good move.

6

One more thing. The way we built up the lowest level is non-standard
because we also allow Eigen vector and row vector types to act as containers.

This is all a mess of bad design decisions, I’m afraid.

  • Bob
7

We don’t want any runtime overhead, so we have to be more careful than
setting flags and branching and what not. That’d be the Java way to
do it, but in C++, we can do much better statically, which makes a huge
difference to run time.

We probably don’t want to copy, but hold a reference. This is meant
to be a short-lived wrapper to be used in scope where the container
being held doesn’t change (the view will track the change if the container
does change).

  • Bob
8

Gotcha, so does that mean we also don’t want the constructor time overhead of constructing a singleton std::vector in the single matrix constructor case? If that’s true, how do we do this without runtime overhead?

I think I’ll submit a pull request with tests that pass with the current behavior and comments that the behavior has a bug while continuing to work with you guys to figure out how to fix the bug.

[edit] pull request here for tests that are okay with buggy behavior: https://github.com/stan-dev/math/pull/465

9

I’m not exactly sure what case you’re talking about.
There are two types involved in these views: return
type and container/scalar type.

Whatever it is, I don’t see how we can avoid construction
overhead. It’s fast and on the stack, but it’s not nothing.
And we want to store references to avoid any copying
during construction.

We’re never constructing singletons for these in the design
patterns sense of singletons. We don’t care about the size
of a vector—if it’s a vector, we just store it. If it’s size
zero or size one, it’s not worth the overhead to test for that
and unpack it, either in code complexity or runtime.

  • Bob
10

Sorry, I don’t mean singleton from GoF. Just the case where there’s a matrix passed in instead of a std::vector of matrices. I think I was proposing something that doesn’t make sense here, so never mind.

So for now I will leave the is_array logic alone and not test it, how does that sound? And then the other thread where you proposed new versions of all of these views for a refactor can continue.

[edit] woops, didn’t see your reply in the other thread. Will go forward with that. Thanks!