Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Adds ability to load quantized weights by de-quantizing them. #965

Merged
merged 11 commits into from
Apr 25, 2018
Merged

Adds ability to load quantized weights by de-quantizing them. #965

merged 11 commits into from
Apr 25, 2018

Conversation

adarob
Copy link
Member

@adarob adarob commented Apr 18, 2018

Works in conjunction with tensorflow/tfjs-converter#87.


This change is Reviewable

@nsthorat
Copy link
Contributor

Thanks for doing this!

Can you go talk to Suharsh and make sure this approach seems reasonable to him? Specifically I want to make sure 0s are 0s (which means you may have to use scale / offset).


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


src/weights_loader.ts, line 128 at r1 (raw file):

      const fetchUrl = filePathPrefix +
          (!filePathPrefix.endsWith('/') ? '/' : '') + filepath;
      requests.push(fetch(fetchUrl, requestOptions));

this should not be removed


Comments from Reviewable

@adarob adarob changed the title Proposal for adding quantization. Please have a look before I write tests. Adds ability to load quantized weights by de-quantizing them. Apr 20, 2018
@adarob
Copy link
Member Author

adarob commented Apr 20, 2018

@nsthorat @pyu10055 this is now ready for review.

@dsmilkov dsmilkov requested review from dsmilkov and caisq April 24, 2018 18:57
@dsmilkov
Copy link
Contributor

Looks great! One comment to reduce number of intermediate allocated arrays, and after that I'm very happy to get this in!


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/weights_loader.ts, line 178 at r3 (raw file):

        let quantizedArray: Float32Array;
        if (quantization.dtype === 'uint8') {
          quantizedArray = Float32Array.from(new Uint8Array(byteBuffer));

no need to have this intermediate Float32Array. Just allocate the final typedArray depending on the logical dtype (Int32Array or Float32Array). JS will give you all zeros if you use the c-tor with the size passed in.

Then iterate directly over the UIn8Array or UInt16Array (depending on the quantized dtype), and do the math directly there.

const quantized = new Uint8Array(byteBuffer); // or UInt16Array
const typedArray = new Float32Array(quantized.length); // or Int32Array
for (let i = 0; i < a.length; i++) {
  typedArray[i] = quantized[i] * scale + min;
}

Comments from Reviewable

@dsmilkov
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/weights_loader.ts, line 178 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

no need to have this intermediate Float32Array. Just allocate the final typedArray depending on the logical dtype (Int32Array or Float32Array). JS will give you all zeros if you use the c-tor with the size passed in.

Then iterate directly over the UIn8Array or UInt16Array (depending on the quantized dtype), and do the math directly there.

const quantized = new Uint8Array(byteBuffer); // or UInt16Array
const typedArray = new Float32Array(quantized.length); // or Int32Array
for (let i = 0; i < a.length; i++) {
  typedArray[i] = quantized[i] * scale + min;
}

Forgot to say , if it's int32, round before you assign to typedArray[i]


Comments from Reviewable

@adarob
Copy link
Member Author

adarob commented Apr 24, 2018

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/weights_loader.ts, line 128 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this should not be removed

Done.


src/weights_loader.ts, line 178 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Forgot to say , if it's int32, round before you assign to typedArray[i]

Done.


Comments from Reviewable

@adarob
Copy link
Member Author

adarob commented Apr 24, 2018

Done.


Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@caisq
Copy link
Collaborator

caisq commented Apr 25, 2018

:lgtm_strong:

Thanks!


Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


src/weights_loader.ts, line 31 at r4 (raw file):

scale: number

Just a suggestion: Having min and scale maybe a little more confusing than having min and max, as it is unclear whether scaling should be applied before or after adding min, and doing it before or after will lead to different results. min and max is less ambiguous in this way. If you insist on doing it this way, please add a doc string to quantization.


src/weights_loader.ts, line 195 at r4 (raw file):

              `Weight ${weightsEntry.manifestEntry.name} has unknown dtype ` +
              `${dtype}.`);

Perhaps be more specific: "weight ... has a dtype unsupported by quantization ..."


src/weights_loader_test.ts, line 470 at r4 (raw file):

quantizationTest

Add another test of a mixed weight manifest, wherein some weights have quantization, others don't.


Comments from Reviewable

@adarob
Copy link
Member Author

adarob commented Apr 25, 2018

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.


src/weights_loader.ts, line 31 at r4 (raw file):

Previously, caisq (Shanqing Cai) wrote…
scale: number

Just a suggestion: Having min and scale maybe a little more confusing than having min and max, as it is unclear whether scaling should be applied before or after adding min, and doing it before or after will lead to different results. min and max is less ambiguous in this way. If you insist on doing it this way, please add a doc string to quantization.

I originally had min/max but needed to switch when we added nudging since min and max is no longer enough information to dequantize. Added doc.


src/weights_loader.ts, line 195 at r4 (raw file):

Previously, caisq (Shanqing Cai) wrote…
              `Weight ${weightsEntry.manifestEntry.name} has unknown dtype ` +
              `${dtype}.`);

Perhaps be more specific: "weight ... has a dtype unsupported by quantization ..."

Done.


src/weights_loader_test.ts, line 470 at r4 (raw file):

Previously, caisq (Shanqing Cai) wrote…
quantizationTest

Add another test of a mixed weight manifest, wherein some weights have quantization, others don't.

Done.


Comments from Reviewable

@pyu10055
Copy link
Collaborator

Reviewed 1 of 2 files at r4.
Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


src/weights_loader.ts, line 187 at r4 (raw file):

        if (dtype === 'float32') {
          dequantize = v => v * quantization.scale + quantization.min;
          typedArray = new Float32Array(quantizedArray.length);

use typeArray = Float32Array.from(quantizedArray, v => v * quantization.scale + quantization.min), same for int32 case.


Comments from Reviewable

@pyu10055
Copy link
Collaborator

:lgtm_strong:


Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


Comments from Reviewable

@pyu10055
Copy link
Collaborator

Reviewed 1 of 2 files at r4.
Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


Comments from Reviewable

@dsmilkov
Copy link
Contributor

:lgtm_strong: Nice work Adam. I think the only outstanding comment is Ping's about using Float32Array.from(). After that I'll merge this in.


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


src/weights_loader.ts, line 187 at r4 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

use typeArray = Float32Array.from(quantizedArray, v => v * quantization.scale + quantization.min), same for int32 case.

+1. This will create one less intermediate array


Comments from Reviewable

@adarob
Copy link
Member Author

adarob commented Apr 25, 2018

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


src/weights_loader.ts, line 187 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

+1. This will create one less intermediate array

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 242f627 into tensorflow:master Apr 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants