Skip to content

Allow 1D vector for w_scale in QLinearConv #6460

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 28, 2024
Merged

Allow 1D vector for w_scale in QLinearConv #6460

merged 7 commits into from
Oct 28, 2024

Conversation

mcollinswisc
Copy link
Contributor

Description

This handles the case that the w_scale is a 1-D vector in the ReferenceEvaluator implementation of the QLinearConv op. It first checks if w_scale is a 1-D tensor. If it is, then this new branch adds dimensions it so that it will broadcast correctly to output channels in scaling the result of the QLinearConv.

Motivation and Context

#6428

Documentation for QLinearConv (https://github.com/onnx/onnx/blob/main/docs/Operators.md#QLinearConv)
states of the input w_scale that:

It could be a scalar or a 1-D tensor, which means a per-tensor/layer or per output channel quantization.

This change ensures the reference evaluator has more complete support of this op. Currently: passing a 1-D tensor for w_scale will raise an error like:

Traceback (most recent call last):
  File "onnx_qlinearconv.py", line 71, in <module>
    ref_result = ref_eval.run(None, {"x": x})
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "env/qlinearconv/lib/python3.12/site-packages/onnx/reference/reference_evaluator.py", line 599, in run
    outputs = node.run(*inputs, **linked_attributes)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "env/qlinearconv/lib/python3.12/site-packages/onnx/reference/op_run.py", line 466, in run
    res = self._run(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "env/qlinearconv/lib/python3.12/site-packages/onnx/reference/ops/op_qlinear_conv.py", line 53, in _run
    R = res * (x_scale * w_scale / y_scale)
        ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ValueError: operands could not be broadcast together with shapes (1,8,24,24) (8,)

This PR adds a unit test that will trigger this error in the old op implementation.

@mcollinswisc mcollinswisc requested review from a team as code owners October 14, 2024 05:53
@mcollinswisc
Add a test on vector w_scale
07ba2bd 
Expands TestReferenceEvaluator.test_qlinearconv with a subTest that has
a 1D vector for w_scale. It's compared against ONNXRuntime, which
already supports this.

Signed-off-by: Maxwell D Collins <mcollins@cs.wisc.edu>
@mcollinswisc
Handle w_scale as a per-output-channel vector
adc795e 
By checking whether it's a 1D vector, and if so expanding the shape so
that it will broadcast to output channels correctly.

Signed-off-by: Maxwell D Collins <mcollins@cs.wisc.edu>
@codecov Codecov
Copy link

codecov bot commented Oct 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.36%. Comparing base (688dad7) to head (729daf2).
Report is 280 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6460      +/-   ##
==========================================
+ Coverage   57.24%   57.36%   +0.11%     
==========================================
  Files         507      507              
  Lines       31442    31502      +60     
  Branches     3541     3544       +3     
==========================================
+ Hits        18000    18072      +72     
+ Misses      12593    12581      -12     
  Partials      849      849

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

justinchuby
justinchuby reviewed Oct 15, 2024
View reviewed changes
@mcollinswisc
Test validation of w_scale shape
155d1d3 
Also fixing the wording on one of the ValueError messages en passant

Signed-off-by: Maxwell D Collins <mcollins@cs.wisc.edu>
@mcollinswisc
Merge branch 'main' into w_scale_1D
61cd627 
Signed-off-by: Maxwell D Collins <mcollins@cs.wisc.edu>
mcollinswisc and others added 2 commits October 18, 2024 10:56
@mcollinswisc
Merge branch 'main' into w_scale_1D
bf8e74a 
Signed-off-by: Maxwell D Collins <mcollins@cs.wisc.edu>
@mcollinswisc
Hard-code expected QLinearConv outputs
40d39bf 
Move test cases for QLinearConv that do not depend on programmatically-
constructed inputs to their own test case that does not require
ONNXRuntime. This is expected to allow the coverage tool be able to tell
that there is test coverage here.

Signed-off-by: Maxwell D Collins <mcollins@cs.wisc.edu>
@xadupre xadupre enabled auto-merge October 21, 2024 08:28
@xadupre xadupre added this pull request to the merge queue Oct 28, 2024
Merged via the queue into onnx:main with commit 9f4b58a Oct 28, 2024
33 checks passed
@mcollinswisc mcollinswisc deleted the w_scale_1D branch October 28, 2024 17:53
@botantony botantony mentioned this pull request May 14, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xadupre xadupre xadupre approved these changes

@justinchuby justinchuby Awaiting requested review from justinchuby

Assignees
No one assigned
Labels
module: reference implementation
Projects
PR Tracker
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mcollinswisc @justinchuby @xadupre @ramkrishna2910