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

replace the weights node name with scaled weights node #2284

Merged
merged 5 commits into from Oct 30, 2019

Conversation

pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Oct 29, 2019

Fixes #2055

When the batchnorm ops share the same weights, the scaled weights calculated by batchnorm folding will generate duplicated nodes.
This fix uses the conv2d op node name as the base to avoid the duplication.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @pyu10055)


tfjs-converter/python/tensorflowjs/converters/fold_batch_norms_test.py, line 144 at r1 (raw file):

        self.assertNotEqual("FusedBatchNorm", node.op)

  def testFoldFusedBatchNormsV3(self):

this seems okay for now but weren't we talking about having some testing infra that lets you define a graph, run the converter, and then execute it (maybe in node) to make sure these things are no-ops?

for now this is cool, can you just make sure this gives identical results for a real model before and after this change?

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 @dsmilkov and @nsthorat)


tfjs-converter/python/tensorflowjs/converters/fold_batch_norms_test.py, line 144 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this seems okay for now but weren't we talking about having some testing infra that lets you define a graph, run the converter, and then execute it (maybe in node) to make sure these things are no-ops?

for now this is cool, can you just make sure this gives identical results for a real model before and after this change?

we have integration tests that verifies the model inference results in the tfjs/integration folder, it requires a new pip released first.

@pyu10055 pyu10055 merged commit 4098e15 into master Oct 30, 2019
@pyu10055 pyu10055 deleted the fix_batch_norm_foding_dupe_nodes branch October 30, 2019 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check failed: st.ok() Non unique node name detected
3 participants