NXP backend: Per-channel quantization of convolution layer #14061
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14061
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Cancelled JobAs of commit 1a3d9e5 with merge base 7e228ee ( NEW FAILURE - The following job has failed:
CANCELLED JOB - The following job was cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
|
Based on a conversation from this PR, I added a feature that uses the implementation. The old PR can be declined. |
StrycekSimon
changed the title
|
@pytorchbot label "release notes: nxp" |
pytorch-bot
bot
added
the
release notes: nxp
robert-kalmar
added
the
module: nxp
| ) | ||
| assert nodes[10].name == "aten_convolution_default" | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Minor: I would move this method up to be 1st, to aling with other tests.
There was a problem hiding this comment.
This is pretty chaotic across our unittest tests. I will make it consistent.
| "QDQDequantizeConverter", | ||
| "QDQPerTensorDequantizeConverter", | ||
| "QDQPerChannelDequantizeConverter", | ||
| "QDQQuantizeConverter", |
There was a problem hiding this comment.
only QDQDequantizer needs to be updated, not QDQQuantizeConverter too?
There was a problem hiding this comment.
Correct, as there are no changes to QDQQuantizeConverter. Per channel quantization scheme is used only for weights and biases, which are inputs - dequantize nodes.
| list[tuple[fx.Node, NodeArgsIdx]] | ||
| | list[tuple[fx.Node, NodeArgsIdx, DerivedQuantizationSpec]] | ||
| ), | ||
| spec: QuantizationSpec | None, |
There was a problem hiding this comment.
just curious, why switch from Optional to | None?
There was a problem hiding this comment.
It's a part of move to Python 3.10 type hints and leaving imports from Typing.
|
|
||
| # pyre-ignore[6]: incompatible parameter type | ||
| annotate_inputs(anchors.inputs, input_act_qspec) | ||
| annotate_weights_or_biases(anchors.weights, weight_qspec) |
There was a problem hiding this comment.
is this function no longer used at all now and can be removed entirely?
There was a problem hiding this comment.
Yes, it is replaced by annotate_inputs().
robert-kalmar
force-pushed
the
upstream/main-nxp/EIEX-486+EIEX-493-upstream-per-channel-quantization-of-convolution
branch
from
02d89fb to
16df359
Compare
StrycekSimon
force-pushed
the
upstream/main-nxp/EIEX-486+EIEX-493-upstream-per-channel-quantization-of-convolution
branch
from
16df359 to
1a3d9e5
Compare
|
The error in Samsung's tests seems to be caused by some problem with downloading resources, and is most likely unrelated. Update: I double-checked it and this error is also present in other PRs... |
robert-kalmar
deleted the
upstream/main-nxp/EIEX-486+EIEX-493-upstream-per-channel-quantization-of-convolution
branch
Summary
Adds per-channel quantization for convolution layer and introduces NodeArgsIdx class to Neutron Quantizer for better handling of indexes to quantized node's args list.
NodeArgsIdx allows selection of nested objects, e.g. an object in a list in node's args list. It also simplifies NeutronAtenQuantizer annotation process by using annotate_inputs() for inputs, weights and biases.
Test plan
The implementation should be covered by either existing or newly added unit tests.
cc @digantdesai @JakeStevens @robert-kalmar, @skywall, @roman-janik-nxp