Skip to content
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

implementing fake_quantize_per_channel_affine #959

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MovGP0
Copy link
Contributor

@MovGP0 MovGP0 commented Mar 15, 2023

this PR contains the following changes:

  • implementation of torch.fake_quantize_per_channel_affine
  • cleanup of AdjustGamma class

@NiklasGustafsson
Copy link
Contributor

Error in all builds:

test\TorchSharpTest\TestTorchTensor.cs(7771,19): error CS0623: Array initializers can only be used in a variable or field initializer. 

@MovGP0
Copy link
Contributor Author

MovGP0 commented Mar 16, 2023

@NiklasGustafsson the Array initializer should have been new double[,,] instead of new double[][][]. have fixed it in a new commit.

Remark: I should build and run tests before committing. However, my local build still doesn't work all the time, so using the CI is my workaround for now 😖

@NiklasGustafsson
Copy link
Contributor

Remark: I should build and run tests before committing. However, my local build still doesn't work all the time, so using the CI is my workaround for now 😖

Understood. Here are test failures:

Failed TorchSharp.TestTensor.TestFakeQuantizePerChannelAffine [22 ms]
  Error Message:
   System.Runtime.InteropServices.ExternalException : !needs_dynamic_casting<func_t>::check(iter) INTERNAL ASSERT FAILED at "../aten/src/ATen/native/cpu/Loops.h":308, please report a bug to PyTorch. 
Exception raised from cpu_kernel at ../aten/src/ATen/native/cpu/Loops.h:308 (most recent call first):

@@ -0,0 +1,96 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a local VS Code file. should it really have been checked in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. it shouldn't. Will remove the file and add an entry to .gitignore.

Haven't found the time to fix the issue with the unit test yet :-/ Trying to analyze & fix it next weekend.

@NiklasGustafsson
Copy link
Contributor

NiklasGustafsson commented Apr 24, 2023

@MovGP0 -- ready to merge? If so, please add something to the top of the RELEASENOTES.md file explaining the API additions. We're working on 0.99.5 now -- planning to release it before the end of the week.

@NiklasGustafsson
Copy link
Contributor

Sorry, I had to make a release of 0.99.5 today.

@NiklasGustafsson
Copy link
Contributor

@MovGP0 -- Ping! Is this ready to merge? It'd be great to get a little one-line blurb on this change for the release notes.

var scales = (torch.randn(2) + 1d) * 0.05d;
var zero_points = torch.zeros(2).to(torch.int32);
var result = torch.fake_quantize_per_channel_affine(x, scales, zero_points, axis: 0, quant_min: 0, quant_max: 255);
Assert.True(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? Is this just because the test is really just looking for an exception in the call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took this code from the official documentation. It's the minimal amount of testing.

Should test the result values, but would need to get rid of the random initializer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either use a Generator instance, or (I do this frequently) just create a tensor from the random values in the PyTorch doc and then the correct results should be right there in the documentation already...

@MovGP0
Copy link
Contributor Author

MovGP0 commented May 17, 2023

Sorry, I had to make a release of 0.99.5 today.

Hey, sorry that I reply this late. Have the championships this and next month and also a new job, so spending all my free time with training or learning 😔 Will probably find time in 4 weeks for further contributions 😊

@NiklasGustafsson
Copy link
Contributor

@MovGP0 -- do you want to keep this PR open?

@NiklasGustafsson NiklasGustafsson marked this pull request as draft October 20, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants