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

fix issue getting number value for oneof field #190

Merged
merged 3 commits into from
Jul 31, 2018
Merged

fix issue getting number value for oneof field #190

merged 3 commits into from
Jul 31, 2018

Conversation

pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Jul 28, 2018

There was a bug with operations that use integer parameters such as Split. These were being given a default float value of 0, so param.f was never evaluating to undefined. Accordingly, the result for all integer parameters was always 0.

Fixes 535


This change is Reviewable

@pyu10055 pyu10055 requested a review from dsmilkov July 28, 2018 00:12
Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Nice catch! Requesting a few more unit tests to make sure I got 'bool' and 'float' covered as well.

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


src/operations/operation_mapper_test.ts, line 105 at r1 (raw file):

      attr: {squeeze_dims: {list: {i: [Long.fromInt(1), Long.fromInt(2)]}}}
    },
    {

Can you add a few more unit tests for ops that have

  1. float param, (with and without default value), such as FusedBatchNorm which has:
attr {
    name: "epsilon"
    type: "float"
    default_value {
      f: 0.0001
    }
  } 
  1. bool param (again with and without default value), such as FusedBatchNorm which as:
attr {
    name: "is_training"
    type: "bool"
    default_value {
      b: true
    }
  }

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 @pyu10055)


src/operations/operation_mapper_test.ts, line 105 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Can you add a few more unit tests for ops that have

  1. float param, (with and without default value), such as FusedBatchNorm which has:
attr {
    name: "epsilon"
    type: "float"
    default_value {
      f: 0.0001
    }
  } 
  1. bool param (again with and without default value), such as FusedBatchNorm which as:
attr {
    name: "is_training"
    type: "bool"
    default_value {
      b: true
    }
  }

added float value, there is already a bool value test.

@pyu10055 pyu10055 merged commit 3a30c27 into master Jul 31, 2018
@pyu10055 pyu10055 deleted the split_bug branch July 31, 2018 22:53
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