refactor(ONNX): replaces getValueList helper with createScalarSublist
#3987
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
|
Hey, @AmosLewis, could you review this since @zjgarvey is out? Zach reviewed a lot of it under #3945, but wanted to split into two stacked PRs to make each final diff smaller. |
|
I'll get a chance to review today, too. But it would be good to get Chi's review too. |
zjgarvey
requested changes
Jan 29, 2025
zjgarvey
reviewed
Jan 29, 2025
bjacobgordon
force-pushed
the
refactor-onnx-replaces-getValueList-helper-with-createScalarSublist
branch
from
2e2300b to
9e57557
Compare
zjgarvey
reviewed
Jan 30, 2025
bjacobgordon
force-pushed
the
refactor-onnx-replaces-getValueList-helper-with-createScalarSublist
branch
from
9e57557 to
1580393
Compare
|
@zjgarvey All the tweaks were super quick, and one of them sparked a few ideas! It took some time, but I think it'll be worth it:
I think there's a ton of opportunity for subsequent PRs to leverage these new helpers to gradually distill the implementation for these conversions! Hoping that it leaves you nitless, but we'll see! Just let me know haha. |
bjacobgordon
changed the title
getValueList helper with createScalarSublistgetValueList helper with createTorchScalarSublist
zjgarvey
requested changes
Feb 1, 2025
There was a problem hiding this comment.
Hey, Jacob.
The PR was in a great state to merge before you made the undiscussed changes. To make this process more efficient, would you please revert those changes so that we can merge the PR as discussed earlier?
This PR is blocking your progress on another PR, so I don't want to spend another day or two going back and forth on these new changes.
bjacobgordon
force-pushed
the
refactor-onnx-replaces-getValueList-helper-with-createScalarSublist
branch
2 times, most recently
from
f61b1e8 to
a751f8c
Compare
…tValueList` helper
- avoids useless declaration and assignment
… `getValueList` helper
…ValueList` helper
…n `getValueList` helper
…alueList` helper
…` helper - `operand`: helper isn't concerned with how the call site things about this - `input`: tells us that it's a parameter passed into this scope, but the implementation eventually tells us that we're treating it like a tensor - `inputTensor`: but not just _any_ tensor, it's a 1D tensor - `input1DTensor`: "input" has been used elsewhere to mean "first operand for an operation", and this function isn't a full-blown op, so we gotta use something else - `given1DTensor`: bingo
…lueList` helper - `operandType`: "operand" is the given tensor - `castedGiven1DTensorType`: can be renamed as validated generic - `some1DTensorType`: meaning "type of some 1D tensor"
- Before: - "get": implies retrieval of some private property - "Value": restatement of the return type `Value` - "List": assumed result of casting the returned instance - After: - "create": contextualizes the need to pass in `rewriter` - "Scalar": contextualizes the opaque return type - "Sublist": the relationship between the first parameter and the returned result
bjacobgordon
force-pushed
the
refactor-onnx-replaces-getValueList-helper-with-createScalarSublist
branch
from
a751f8c to
db12b36
Compare
bjacobgordon
changed the title
getValueList helper with createTorchScalarSublistgetValueList helper with createScalarSublist
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.
A preliminary refactor to support #3945