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.

Sign up for GitHub

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

Jump to bottom

Add broadcasting functionality for Div and Sub ops. #17123

Merged

andrehentz merged 9 commits into tensorflow:master from hovhannesgithub:div-sub-broadcasting

Apr 12, 2018

Contributor

hovhannesgithub commented Feb 19, 2018

Hi,
Added broadcasting functionality for Div and Sub ops following the examples of Add and Mul.
Regards,
Hovhannes


          Add broadcasting functionality fro Div and Sub ops.

672ec27

googlebot commented Feb 19, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot added the cla: no label

Contributor Author

hovhannesgithub commented Feb 19, 2018

I signed it!

googlebot commented Feb 19, 2018

CLAs look good, thanks!

googlebot added cla: yes and removed cla: no labels

martinwicke requested a review from aselle

February 20, 2018 17:35

andrehentz self-requested a review

February 22, 2018 16:22

andrehentz reviewed

View reviewed changes

Contributor

andrehentz left a comment

Thanks for doing this. Very nice, apart from the complications of quantization.

tensorflow/contrib/lite/kernels/internal/optimized/optimized_ops.h Outdated

+              // that handles broadcasting as the base case. The code generator would then
+              // generate max(D1, D2) nested for loops.
+              // TODO(benoitjacob): BroadcastDiv is intentionally duplicated from
+              // reference_ops.h. Once an optimized version is implemented and NdArrayDesc<T>

Contributor

andrehentz Feb 22, 2018

Please make sure we are within 80 columns everywhere.

Contributor Author

hovhannesgithub Feb 26, 2018

I checked this file and maximum line length is <=80characters. Nevertheless there was lines >80 characters in sub_test.cc and div_test.cc which I have fixed.

tensorflow/contrib/lite/kernels/internal/optimized/optimized_ops.h Outdated

+                }
+              }
+              // legacy, for compatibility with old checked-in code

Contributor

andrehentz Feb 22, 2018

Since this function didn't exist before you don' t need to keep a legacy version around.

tensorflow/contrib/lite/kernels/internal/optimized/optimized_ops.h Outdated

+                        const int32 unclamped_result =
+                            output_offset +
+                            MultiplyByQuantizedMultiplierSmallerThanOne(
+                                input1_val / input2_val, output_multiplier, output_shift);

Contributor

andrehentz Feb 22, 2018

There's an accuracy issue here as the integer division will truncate a lot of values.

Leaving that aside for a second, the calculation doesn't seem correct either (quantization is hard!). The quantized multiplier is s1s2/sout so the result here is
s1s2/souti1/i2
but it should be s1/s2i1/i2/sout, right?

Again, quantization is hard, so I'll be OK if you prefer to leave quantization for a subsequent PR.

One solution is to calculate a quantized multiplier that is always greater than 1:
qm = (s1> s2sout) ? qmul(s1/s2/sout) : qmul(s2sout/s1)
and pass a hint as to whether the numerator or the denominator needs scaling
return denominator_needs_scaling ? q1 / (q2 * qm) : (q1 * qm) / q2;

Contributor Author

hovhannesgithub Feb 26, 2018

Please can you provide more details or link to docs so I can understand quantized ops better.

Contributor

andrehentz Feb 27, 2018

The quantization scheme used in TF Lite is similar to gemmlowp's: https://github.com/google/gemmlowp/blob/master/doc/quantization.md

Maybe we can leave the quantization for a separate PR? I'd be happy to approve the other changes, and then we can have someone with more quantization experience review the other PR.

tensorflow/contrib/lite/kernels/internal/optimized/optimized_ops.h Outdated

+                }
+              }
+              // legacy, for compatibility with old checked-in code

Contributor

andrehentz Feb 22, 2018

here too: no need for legacy versions since this didn't exist before your changes.

tensorflow/contrib/lite/kernels/internal/optimized/optimized_ops.h Outdated

+                }
+              }
+              // legacy, for compatibility with old checked-in code

Contributor

andrehentz Feb 22, 2018

same here for legacy (there are more of those in the other file)

hovhannesgithub and others added 2 commits

February 26, 2018 10:52


          Remove code that was written for compatibility with old checked-in co…

f1f70ef

…de. Update code to have 80 characters per line.


          Merge branch 'master' into div-sub-broadcasting

cd0a84c

protoget added stat:awaiting response tag:awaiting response labels

hovhannesgithub added 2 commits

March 7, 2018 09:14


          Remove quantized versiaon of Div till fixing it.

82e34cd


          Merge branch 'div-sub-broadcasting' of https://github.com/hovhannesgi…

9db9283

…thub/tensorflow into div-sub-broadcasting

andrehentz approved these changes

View reviewed changes

andrehentz added the kokoro:force-run label

kokoro-team removed kokoro:force-run labels

jpienaar pushed a commit to jpienaar/tensorflow that referenced this pull request


          Broadcast Sub and Div from tensorflow#17123 except for quantization.

143f358

PiperOrigin-RevId: 189216312

sb2nov pushed a commit that referenced this pull request


          The Quantized BroadcastSub portion of #17123

4e5900e

PiperOrigin-RevId: 189776376

tensorflowbutler added the stale label

Member

tensorflowbutler commented Mar 25, 2018

It has been 14 days with no activity and the awaiting response label was assigned. Is this PR still valid? Assigning the stalled label. Please comment to reassure me that this is still being worked on.


          Merge branch 'master' into div-sub-broadcasting

6de04d6

Contributor Author

hovhannesgithub commented Mar 28, 2018

I fixed conflicts and committed them, so please remove "stalled" label

andrehentz added kokoro:force-run and removed stale labels

kokoro-team removed the kokoro:force-run label

tensorflowbutler removed the stat:awaiting response label

Contributor Author

hovhannesgithub commented Apr 10, 2018

Hi,
Is there anything that I should do to make this pull request be merged.
Regards,
Hovhannes

Contributor

jhseu commented Apr 12, 2018 •

edited

Can you look at the test failures?

In file included from tensorflow/contrib/lite/kernels/pooling.cc:26:
./tensorflow/contrib/lite/kernels/internal/reference/reference_ops.h:1415:6: error: redefinition of 'BroadcastDiv'
void BroadcastDiv(const T* input1_data, const Dims<4>& input1_dims,
     ^
./tensorflow/contrib/lite/kernels/internal/reference/reference_ops.h:1347:6: note: previous definition is here
void BroadcastDiv(const T* input1_data, const Dims<4>& input1_dims,

hovhannesgithub and others added 2 commits

April 12, 2018 10:04


          Remove redefined BroadcastDiv function

b79de28


          Merge branch 'master' into div-sub-broadcasting

db3a285

Contributor Author

hovhannesgithub commented Apr 12, 2018

Thanks jhseu for help.


          Fixe merge issue

09ab7fc

andrehentz added the kokoro:force-run label

kokoro-team removed the kokoro:force-run label

andrehentz merged commit eaaee5d into tensorflow:master

hovhannesgithub deleted the div-sub-broadcasting branch

April 13, 2018 05:18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment