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

Fixed decodeWeights bug and added quntization support #1219

Merged
merged 3 commits into from
Aug 13, 2018
Merged

Conversation

pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Aug 11, 2018

Description

Fixed issue tensorflow/tfjs#591


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

@pyu10055 pyu10055 requested a review from caisq August 11, 2018 03:02
Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Thanks for PR!

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055 and @caisq)


src/io/io_utils.ts, line 91 at r1 (raw file):

${quantization.dtype}.

It would be good to say what the supported quantization dtypes are in this error message.


src/io/io_utils.ts, line 108 at r1 (raw file):

 quantization

This should be just 'dtype', not 'quantization dtype', right?


src/io/io_utils.ts, line 114 at r1 (raw file):

const byteBuffer = buffer.slice(offset, offset + size * dtypeFactor);

Thanks for fixing this. Can you add a unit test to cover the previously-failing cases?

Copy link
Collaborator Author

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @pyu10055)


src/io/io_utils.ts, line 91 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
${quantization.dtype}.

It would be good to say what the supported quantization dtypes are in this error message.

Done.


src/io/io_utils.ts, line 108 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
 quantization

This should be just 'dtype', not 'quantization dtype', right?

Done.


src/io/io_utils.ts, line 114 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
const byteBuffer = buffer.slice(offset, offset + size * dtypeFactor);

Thanks for fixing this. Can you add a unit test to cover the previously-failing cases?

the original tests have a mix type test, which I updated its bool data to be odd and failed the original code.

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)


src/io/io_utils.ts, line 108 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Done.

Ah - cool! Sorry I missed that subtle change.

@pyu10055 pyu10055 merged commit 582aa93 into master Aug 13, 2018
@pyu10055 pyu10055 deleted the decode_weights branch August 13, 2018 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants