-
Notifications
You must be signed in to change notification settings - Fork 52
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
Convert AbstractTensor from struct to class #2967
Conversation
`AbstractTensor` now has many methods and as of #2933 it is subclassed into `TaggedAbstractTensor` which has additional methods. Also, there is no longer just a single vector attribute `domain` but also `info`. Instead of requiring the user to manage those vectors and keep them the same length, instead this PR makes them `protected` and uses the existing accessors to adjust them, as suggested in #2963 (comment).
!build |
Is the build failure related? |
I am not sure yet, but I doubt it. I am also seeing this failure on #2913 which doesn't include this change. |
std::unordered_set<Tag>& getTags(int64_t i) { | ||
i = wrapDim(i, (int64_t)this->size()); | ||
return this->info_[i].tags; |
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.
I added this non-const accessor so that we can manually insert and erase tags for a single axis.
I am surprised that this PR just work, because I remembered that there are more direct uses of |
Yeah I thought it was going to be tougher too but the only places
Only |
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.
LGTM, just want to see CI green for double confirming.
!build |
Rebuilding following merging of #2969 |
AbstractTensor
now has many methods and as of #2933 it is subclassed intoTaggedAbstractTensor
which has additional methods. Also, there is no longer just a single vector attributedomain
but alsoinfo
. Instead of requiring the user to manage those vectors and keep them the same length, this PR makes themprotected
members of a class and uses the existing accessors to adjust them, as suggested in #2963 (comment).