-
Notifications
You must be signed in to change notification settings - Fork 64
Improve checks in GenericPattern matcher #2155
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
Conversation
shubhambhokare1
commented
Apr 1, 2025
•
edited
Loading
edited
- Support None inputs
- Add support for allow_extra_inputs
- TODO: Add support for allow_extra_attributes
❌ 8 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
@@ -53,6 +58,9 @@ | |||
) | |||
for a, b in zip(graph_node.outputs, pattern_node.outputs): | |||
self._bind(b, a) | |||
|
Check warning
Code scanning / lintrunner
EDITORCONFIG-CHECKER/editorconfig Warning
@@ -53,6 +58,9 @@ | |||
) | |||
for a, b in zip(graph_node.outputs, pattern_node.outputs): | |||
self._bind(b, a) | |||
|
Check warning
Code scanning / lintrunner
RUFF/W293 Warning
See https://docs.astral.sh/ruff/rules/blank-line-with-whitespace
Is there a plan to merge GenericPattern with Pattern? |
Any unit tests? |
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
onnxscript/rewriter/generic_pattern.py:42
- In the init method, the check requires graph_node.inputs to be strictly greater than pattern_node.inputs when allow_other_inputs is true, while _match_backward only checks if it is not less. Consider aligning these conditions (e.g., using '>=' in both places) to ensure consistent behavior.
if pattern_node.allow_other_inputs:
onnxscript/rewriter/generic_pattern.py:42
- The code uses the property 'allow_other_inputs' while the PR description refers to 'allow_extra_inputs'. Consider using a single term for clarity across the code and documentation.
if pattern_node.allow_other_inputs:
Closing for now, as we are not focusing on improving the generic pattern matcher |