-
-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow the var views to compound multiple expressions and remove var.coeff() callbacks #2064
Conversation
… make a specialization of vari_value that accepts references
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Marking this as WIP because I also need to sort out how to assign a |
In my opinion that is fine. We don't want to (and can't) support
Nice. However I believe a number of open questions will appear regarding this implementation. |
I agree with the
Yeah. I think one solution could be to make all of it's constructors private. Then since |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
I am a bit hesitant, but let's say that is fine. To avoid the adhoc method, I think it might be possible to do something similar we have in
I don't see making things hidden enough a problem. We can always declare member functions private or move stuff in internal namespace. What I mean is things like: How do we make the interface with varis containing references? There are two main groups of accesses to vari (that I see): a) trough the var (calls to .adj() and .val(), var constructors and assignment operators) In both of this cases the accessing class either needs to restricts itself to only accessing virtual functions or knowing exact type in the vari (is it For the Eigen operations we already have rev implementations templated with argument types. I think we need to do the same for scalar ones in order to use the references. I am not so sure about accesses trough var. It already knows exact type of the vari, but ... does it need to? Right now that makes calls to Instead we could have assignments just copy the vari pointer, but |
Had to read your comment a few times to get a full grasp of the issue and those are good points.
I think this is v possible. By the Eigen operations do you mean the math ops that just take in an eigen type? Like how we went from
This one kind of worries me. I'd have to sit down and play around with this to get a better grasp of it. For coefficient access, If we want to think about this more why don't we just do the hard copy for now and get rid of the For the |
Oh actually saying that out loud I think we essentially need to do that before we have a |
Yeah, since we are talking reverse, either the scalar is var or the type is var_value.
I think we can start with templating, before trying more virtual calls. Than we can see what needs to be made virtual for this to work and benchmark to see if virtual version is faster or slower.
Well, just using tem in expression
I think it is not that large change so both can be done in the same PR. If you end up doing it, you can structure PRs as you want.
Yes. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
…-dev/math into feature/multi-expression-vari-views
This looks mostly good now. Although I think we should handle the arena matrix by adding You can derive it or use values from traits specialization for the |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
So I had a go at this before I did the |
If that is the case, no need to specialize all the EIgen internal structures. However the current implementation could be buggy in some edge cases. Can you instead make a |
…ype inherits from MapBase
…4.1 (tags/RELEASE_600/final)
…-dev/math into feature/multi-expression-vari-views
* @ingroup type_trait | ||
*/ | ||
template <typename T> | ||
struct is_eigen_map_base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anywhere. Remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah! lol I was doing something else and slammed this in there
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
This PR allows for
var<matrix>
types to chain together multiple slicing methods such asIt also has a specialization of
vari_value<T>
whenT
is a reference to a scalar type. This specialization removes the callbacks that were needed in the last implementation of thevar_value.coeff()
methods.While I was working on the
var<Matrix>
indexing methods in this stan branch I realized I needed to be able to write compound expressions such asa_var_matrix.rowwise().reverse()
.reverse()
was needed in particular because stan's indexing method for code such asuses an
index_min_max(5, K)
when performing indexing, but this name is dubious! The stan language will useindex_min_max(5, K)
even if we don't know whetherK
is greater than the starting index! In the case of a decreasing index we need to be able to call eigen's.reverse()
function so that themin_max
index can grab the appropriate slice and then reverse the order.This PR adds those methods and allows them to be chained together. To do that I had to also change
is_eigen
andplain_type_t
. Currentlyis_eigen
checks to see whether or not a pointer to a typeT
is convertible toEigen::EigenBase<T>*
. But some Eigen expressions such asVectorWiseOp
don't actually inherit fromEigenBase
sois_eigen<VectorWiseOp<T>>
fails (see line 156 of theVectorWiseOp
code link).VectorWiseOp
also does not have aPlainObject
trait soplain_type_t
fails for these types. This means I had to change both of these definitions a bit whereis_eigen
now checks whether the typeT
is pointer convertible toEigenBase<T>*
or ifT
has a trait namedExpressionTypeNestedCleaned
. A similar thing is used forplain_type_t
where we detect whether the class has either aPlainObject
trait or aExpressionTypeNestedCleaned
trait and report back either. (@t4c1 this feels a bit ad-hoc so if you have a nicer way to deduce these I'm all ears!)Another notable change in this PR is that there is now a
vari_value<T>
specialization for whenT
is a reference type. Having this specialization allows us to remove the callbacks that were needed when slicing avari<matrix>
type to get back a single coefficient.Tests
This adds additional tests in
var_test
to check if compound slicing expressions work and if their values and adjoints match up to the original values.Side Effects
Yes, I worry that the
vari_value<T>
specialization that accepts references is not defensive enough. Right now I have the specialization marked asfinal
which should stop anyone from deriving a class from it. This class should really only be used invar_value
, is there some way to allow that?Release notes
Adds views to
vari_value<Matrix>
types that allow for compound slicing expressions.Checklist
Math issue How to add static matrix? #1805
Copyright holder: Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the 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 basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested