[EIEX-885] Add log support using new Neutron flow#20145
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20145
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 1 Cancelled JobAs of commit 99747d3 with merge base 2759ef1 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
robert-kalmar
added
module: nxp
| # Use 256 elements so that, after quantization to uint8, the input can | ||
| # cover the full discrete range [0, 255]. | ||
| # The dataset is generated as a linear float ramp and later quantized, | ||
| # which effectively exercises all uint8 values. |
There was a problem hiding this comment.
Nit: uint8 -> int8 (2 occurrences)
There was a problem hiding this comment.
I don’t want to use int8 or the range [-128, 127] for values from a log input—it doesn’t look right to me. Is it OK if I remove both uint8 references but keep the [0, 255] range?
There was a problem hiding this comment.
I don't understand. What do you mean you don't want to use int8?
Our quantizer always quantizes to int8 (not uint8 as your comment suggests), so the quantized values are always in the range [-128, 127]. It's not about choice.
There was a problem hiding this comment.
Mathematically, you cannot calculate a log for input values ≤ 0. It doesn’t seem correct to mentioned negative inputs in this log test.
There was a problem hiding this comment.
Your comment in code and my comments in this thread are talking about quantized data. Our quantizer quantizes to int8 whether we like it or not.
This quantized data corresponds to real float data, that is not effected by the quantization type. The quantized value -42 can correspond to the float value 3.14, which is perfectly fine for the log.
Also, with the "full range" dataset you are using, you are also testing with the float value 0.0 (it likely corresponds to the quantized value -128). This is because the zero_point data type is equal to the quantization datatype, so the float 0.0 is always representable precisely using quantized values. So you are using an invalid log input anyway. I'm guessing the output is NaN for that particular input value.
There was a problem hiding this comment.
In this case, I agree with you :) I will fix it.
Sorry for the misunderstanding. I was still thinking about the numbers processed on board, but not in the high-level framework.
Thank you for the clarification.
| class ConvBlocksWithLogModule(torch.nn.Module): | ||
| def __init__(self, conv_in_channels: int = 3): | ||
| super().__init__() | ||
| self.block1 = torch.nn.Sequential( | ||
| torch.nn.Conv2d( | ||
| in_channels=conv_in_channels, | ||
| out_channels=3, | ||
| kernel_size=(2, 2), | ||
| stride=(2, 2), | ||
| ), | ||
| torch.nn.ReLU(), | ||
| ) | ||
| self.block2 = torch.nn.Sequential( | ||
| torch.nn.Conv2d( | ||
| in_channels=conv_in_channels, | ||
| out_channels=10, | ||
| kernel_size=(2, 2), | ||
| stride=(2, 2), | ||
| ), | ||
| torch.nn.ReLU(), | ||
| ) | ||
|
|
||
| def forward(self, x): | ||
| x = self.block1(x) | ||
| x = x.clamp_min(1e-6).log() | ||
| return self.block2(x) |
There was a problem hiding this comment.
No change needed here, but such a complex test model is counterproductive. We prefer simple models (1 to 3 ops at most) which focus on some specific case as efficiently as possible. Here fore example, the Relu, Clamp and Relu have no effect on the testing conditions, but they make the test more complex and on lines 88-89, the test may require changes if the support for the related ops is modified (e.g. Vasek's Conv PR is merged).
There was a problem hiding this comment.
If the convolution is changed to support batch sizes greater than 1, this test will fail. I was inspired by the abs operator, and this test is based on it. However, I would prefer to remove it completely, if it is acceptable to have just one test for the log.
There was a problem hiding this comment.
Sure, you can remove this test and keep only 1 (convolution and node formats don't affect the log operator). Just make sure the other test uses the use_qat fixture (to test both PTQ and QAT). And also parametrizing the input shape (to test with at least a couple of different shapes and ranks) wouldn't hurt.
| return self.block2(x) | ||
|
|
||
|
|
||
| class TestLog: |
There was a problem hiding this comment.
Please add a test using QAT. (All operators should have at least 1)
| equal to `high`, with increments depending on the total number of elements. | ||
| """ | ||
|
|
||
| def __init__(self, num_samples=2, low=0.0, high=1.0): |
There was a problem hiding this comment.
Nit: Perhaps it would be more useful to have the default range include negative values (for example if we want to use this to test relu/prelu/...). How about -1 to 1?
There was a problem hiding this comment.
Ok for me. Will change
3 participants
Summary
NXP backend: Enable aten.log with new Neutron flow.
Test plan
Tests provided.
cc @robert-kalmar @JakeStevens @digantdesai @rascani