Refactor indexing functions #3011
Conversation
bbbales2
left a comment
bbbales2
left a comment
There was a problem hiding this comment.
Looks good. I left two notes but I think they were minor
|
I skimmed over the code and I think everything looks okie dokie! I'll have another look tonight |
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
Looks so much nicer 🙏 couple comments but overall looks good!
| inline void assign(Vec1&& x, | ||
| const cons_index_list<index_multi, nil_index_list>& idxs, | ||
| const Vec2& y, const char* name = "ANON", int depth = 0) { | ||
| inline void assign(Vec1&& x, const Vec2& y, const char* name, | ||
| const index_multi& idx) { |
There was a problem hiding this comment.
Is there a reason to remove the default value of anon?
There was a problem hiding this comment.
[optional] I think with this change we could also change index_multi so it just holds a const& to the inner std vector
There was a problem hiding this comment.
Once you go variadic, default value isnt really that useful I guess, the only use is assign(x,y) without indexing. The name is always set in code-gen.
src/stan/model/indexing/assign.hpp
Outdated
| inline void assign(Vec1&& x, | ||
| const cons_index_list<index_omni, nil_index_list>& idxs, | ||
| Vec2&& y, const char* name = "ANON", int depth = 0) { | ||
| inline void assign(Vec1&& x, Vec2&& y, const char* name, index_omni) { |
There was a problem hiding this comment.
Just because it's how we write other functions where we don't use a param
| inline void assign(Vec1&& x, Vec2&& y, const char* name, index_omni) { | |
| inline void assign(Vec1&& x, Vec2&& y, const char* name, index_omni /* idx */) { |
src/stan/model/indexing/assign.hpp
Outdated
| inline void assign(Mat1&& x, | ||
| const cons_index_list<index_omni, nil_index_list>& idxs, | ||
| Mat2&& y, const char* name = "ANON", int depth = 0) { | ||
| inline void assign(Mat1&& x, Mat2&& y, const char* name, index_omni) { |
There was a problem hiding this comment.
same thing with /* idx */
src/stan/model/indexing/assign.hpp
Outdated
| * tail assignment. | ||
| */ | ||
| template <typename StdVec, typename Idx, typename U, | ||
| template <typename StdVec, typename U, typename... Idxes, |
There was a problem hiding this comment.
Typo?
| template <typename StdVec, typename U, typename... Idxes, | |
| template <typename StdVec, typename U, typename... Idxs, |
src/stan/model/indexing/assign.hpp
Outdated
| require_all_std_vector_t<T, U>* = nullptr, | ||
| require_not_same_t<Idx1, index_uni>* = nullptr> | ||
| inline void assign(T&& x, U&& y, const char* name, const Idx1& idx1, | ||
| const Idxes&... idxes) { |
| stan::math::check_range("vector[multi] assign", name, x_size, | ||
| idxs.head_.ns_[i]); | ||
| x_idx[i] = idxs.head_.ns_[i] - 1; | ||
| if (likely(x_set.insert(idx.ns_[i]).second)) { |
There was a problem hiding this comment.
I wonder if we should remove the likely here? I've been writing things lately that have a lot of repeated values so idk it's a judgement call
| } | ||
| } | ||
| for (int i = assign_cols - 1; i >= 0; --i) { | ||
| if (likely(x_idx[i] != -1)) { |
There was a problem hiding this comment.
Same here for likely()
|
The test error popping up now is legit: https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/PR-3011/12/pipeline Seems like a missing signature? |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
# Conflicts: # Jenkinsfile
Submission Checklist
./runTests.py src/test/unitmake cpplintSummary
This is a copy of #3010, just moved to original repo to make Jenkins formatting bot work.
Refactors indexing functions to simplify them including:
lvalue.hpptoassign.hppandlvalue_varmat.hpptoassign_varmat.hppto match functions implemented in themrvalue_returnmetaprogramrvalueandassignfunctions:cons_index_liststructure with (possibly variadic) argument list of indices. This makescons_index_listand related functions redundant so they are removed.Intended Effect
The generated code that uses indexing is more readable.
How to Verify
The changes are tested by changed tests.
Side Effects
This changes the interface of indexing functions. So it needs to be merged together with changes in stanc that will generate code according to the new interface.
Documentation
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): Tadej Ciglarič
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: