-
Notifications
You must be signed in to change notification settings - Fork 92
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
Transforms as operators #452
Merged
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
luitjens
requested changes
Jul 19, 2023
/blossom-ci |
1 similar comment
/blossom-ci |
f599d12
to
9037920
Compare
9037920
to
2cb558b
Compare
/blossom-ci |
1 similar comment
/blossom-ci |
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 PR marks a major breaking change in MatX syntax. Previously we had a clear distinction of operators vs transforms such that they could not be combined. To do an FFT convolution, the user would write:
This code is taken directly from the FFT convolution sample. While the code is readable, anyone who has done an FFT convolution in another language like Python would usually write it as a single line. This was not possible in MatX due to transforms having the requirement of being on their own line. The other thing to notice is that transforms have a different syntax than regular operator expressions; in one case the output is a parameter to a function, while in the other case the output is done using the assignment operator. Having the output as a function parameter has a couple of benefits, such as deducing what to do based on the output, but it has far more downsides.
This PR introduces "transforms as operators". The general idea is that if the user wants to, they can use transforms inside of any operator expression. The FFT convolution above changes to:
Someone familiar with signal processing will immediately recognize the convolution. Besides readability, this syntax has many benefits:
Under the hood, the feature works by detecting whether a transform is being used as part of a larger expression, and, if so, a temporary async-allocation is made scoped to the lifetime of the operator. In our testing we've found the overhead from these allocations are small (~1%), depending on the size of the problem. Users not wanting the extra overhead can continue to use the old multi-line method, but converted to the new assignment syntax:
In this case MatX detects that these are "simple" transforms, and reverts to the fast-path without any asynchronous allocations, but maintaining the newer syntax.
Transforms that return multiple types are required to be standalone, as they are in Python:
mtie
is used to "tie" multiple values together similar tostd::tie()
.mtie
is needed to overload the assignment operator.Feedback is always welcome!
Closes #450