Introduce TaggedAbstractTensor #2933
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This generalizes `AbstractTensor` to the templated struct `AbstractTensorWithInfo<Info>`. Related to #2913.
|
!build |
zasdfgbnm
reviewed
Sep 11, 2024
This cleans up the additional constructors for TaggedAbstractTensor quite a bit.
zasdfgbnm
reviewed
Sep 16, 2024
| TaggedAbstractTensor( | ||
| std::initializer_list<AbstractId> domain, | ||
| std::initializer_list<std::initializer_list<Tag>> tag_sets) | ||
| : AbstractTensorWithInfo<TagSetInfo<Tag>>( |
There was a problem hiding this comment.
nit:
Suggested change
| : AbstractTensorWithInfo<TagSetInfo<Tag>>( | |
| : AbstractTensorWithInfo( |
There was a problem hiding this comment.
I thought I could get rid of these also but clang complains if I even remove Tag here. 🤷♂️
| std::initializer_list<std::initializer_list<Tag>> tag_sets) | ||
| : AbstractTensorWithInfo<TagSetInfo<Tag>>( | ||
| domain, | ||
| {tag_sets.begin(), tag_sets.end()}) {} |
There was a problem hiding this comment.
nit:
Suggested change
| {tag_sets.begin(), tag_sets.end()}) {} | |
| tag_sets) {} |
There was a problem hiding this comment.
This actually didn't work for me because although there is an unordered_set ctor for TagSetInfo, there's no vector<unordered_set<Tag>> ctor for vector<TagSetInfo>.
|
!build |
Fixes test failure in AbstractTensorTest.Reorder caused by operator== returning false although the vector<AbstractId> given exactly matched domain. Previously these were automatically converted to AbstractTensor but I guess this no longer works given the new constructors.
jacobhinkle
commented
Sep 17, 2024
zasdfgbnm
approved these changes
Sep 17, 2024
jacobhinkle
added a commit
that referenced
this pull request
Sep 19, 2024
`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).
jacobhinkle
added a commit
that referenced
this pull request
Sep 19, 2024
`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, this PR makes them `protected` members of a class and uses the existing accessors to adjust them, as suggested in #2963 (comment).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This generalizes
AbstractTensorto the templated structAbstractTensorWithInfo<Info>and introduces a special case subclassTaggedAbstractTensor<Tag>. This can be used by passing an enum class forTag, and holds anunordered_set<Tag>for each dimension. Merging and swizzling unions these sets, and split duplicates the set.Note that a lot of code had to be moved out of the cpp into the header because of templatization. However, there are no changes to the
Dispatch*classes. TheAbstractTensorWithInfomethods like split, merge, swizzle, etc. are just changed to add calls toInfo::merge.Related to #2913, which specializes this as
using AbstractMatmulTensor = TaggedAbstractTensor<MatmulDimRole>.