Add docstring correlation cost #514

PyExtreme · 2019-09-16T13:26:32Z

Fix #509

I have added the documentation for the CorrelationCost function and also made the correlation_cost as private.

I am new to open source and this is my first contribution to the community. I would appreciate any reviews.

Thanks

…d-docstring-correlation_cost

PyExtreme · 2019-09-16T14:00:22Z

@googlebot I signed it!

googlebot · 2019-09-16T14:00:27Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

seanpmorgan · 2019-09-16T15:03:44Z

tensorflow_addons/layers/optical_flow.py

+    FlowNet Learning Optical Flow with Convolutional Networks (Fischer et al.) r
+
+    Following are the parameters it takes:
+        input_a: A `Tensor` of the format specified by `data_format`. 


Hi @PyExtreme! Thanks for the contribution! Would you mind adapting this doc string parameters to match the __init__ args that are used to construct this layer. For example you'll notice input_a and input_b are passed in on the Layers __call__ method. There needs to be some slight changes since this doc string is for the Layer's instantiation.

seanpmorgan · 2019-09-16T15:05:45Z

tensorflow_addons/layers/optical_flow.py

+
+        data_format: Specifies the data format. 
+        Possible values are: 
+            "NHWC" float [batch, height, width, channels] 


Also you'll want to adapt this to the keras style of channel_first or channel_last

Sure @seanpmorgan , will make these changes.

Thanks for the help.

WindQAQ

For the documentation of _correlation_cost, I wonder if we could directly assign it from the cpp op. Maybe _correlation_cost.__doc__ = op.__doc__ works. I'm not so familiar with SWIG, @facaiy is it possible to do this?

PyExtreme · 2019-09-16T18:40:22Z

For the documentation of _correlation_cost, I wonder if we could directly assign it from the cpp op. Maybe _correlation_cost.__doc__ = op.__doc__ works. I'm not so familiar with SWIG, @facaiy is it possible to do this?

Hi @WindQAQ , Could you please have a look at my changes? Is there anything more required?

fsx950223 · 2019-09-17T01:27:03Z

Could I review this pull request? @WindQAQ @facaiy @seanpmorgan

seanpmorgan · 2019-09-17T01:34:50Z

Could I review this pull request? @WindQAQ @facaiy @seanpmorgan

Absolutely! We're always looking for more reviewers (should probably add that to CONTRIBUTING.md). It still requires a maintainer to merge but its super helpful.

fsx950223 · 2019-09-17T01:40:15Z

tensorflow_addons/layers/optical_flow.py

-    with tf.name_scope(name or "correlation_cost"):
-        op_call = _correlation_cost_op_so.correlation_cost
+    with tf.name_scope(name or "_correlation_cost"):
+        op_call = _correlation_cost_op_so._correlation_cost


It seems _correlation_cost_op_so has no attribute '_correlation_cost'. Have you test the code on the local environment? And I believe correlation_cost is more suitable for name scope.

addons/tensorflow_addons/layers/optical_flow_test.py

Line 22 in f6d9f31

from tensorflow_addons.layers.optical_flow import correlation_cost, CorrelationCost

The function name correlation_cost should be changed to _correlation_cost.

Hi @fsx950223 , we won't be able to import it still. I will change the name scope but I am not sure how to go with the test file so as to run the test.
One way is to make correlation_cost nested in some other function and calling the outer function. But this will not be private, however, easy to import in test.
Second way is to add _correlation_cost function in the test file also.
Alternatively, I think, if somehow, correlation_cost function can be added as a private function to the CorrelationCost class and then we can call call it.

fsx950223 · 2019-09-17T02:26:27Z

Maybe you should rewrite the test case too.

seanpmorgan · 2019-09-17T02:32:54Z

Maybe you should rewrite test case too.

Could you be more specific in your reviews and describe why you think this should be done. Also this PR should really stick to the docstring as it's addressing a posted issue.

fsx950223 · 2019-09-17T02:39:15Z

Maybe you should rewrite test case too.

Could you be more specific in your reviews and describe why you think this should be done. Also this PR should really stick to the docstring as it's addressing a posted issue.

addons/tensorflow_addons/layers/optical_flow_test.py

Line 22 in f6d9f31

    
           from tensorflow_addons.layers.optical_flow import correlation_cost, CorrelationCost

The function name correlation_cost should be changed to _correlation_cost. And sometimes test case is an example for developers. It should not use private functions.

seanpmorgan · 2019-09-17T18:50:16Z

For the documentation of _correlation_cost, I wonder if we could directly assign it from the cpp op. Maybe _correlation_cost.__doc__ = op.__doc__ works. I'm not so familiar with SWIG, @facaiy is it possible to do this?

Unfortunately looks like no (at least for the way we're currently doing it):
https://colab.research.google.com/drive/1g9XmQbT-gDuAUyVkQSqgFLZVCEvEmSIJ

seanpmorgan · 2019-09-17T19:06:22Z

Hi @PyExtreme I've made a small commit on your branch just because this is blocking a few things (and I should have taken care of this a week or two ago when Yan pointed it out). Can you please review and let me know if it makes sense.

@fsx950223 Thank you very much for your review. After your clarification I understand. I agree on the namespacing. As far as not testing the private method I think that's a good idea but not essential. Renaming the op should be enough to indicate to users that it's discouraged (but not impossible) to use that Op by itself. Happy to accept a PR which modifies those test cases separately though.

EDIT ----
Also don't worry about the CLA we can override it.

WindQAQ · 2019-09-17T21:22:32Z

For the documentation of _correlation_cost, I wonder if we could directly assign it from the cpp op. Maybe _correlation_cost.__doc__ = op.__doc__ works. I'm not so familiar with SWIG, @facaiy is it possible to do this?

Unfortunately looks like no (at least for the way we're currently doing it):
https://colab.research.google.com/drive/1g9XmQbT-gDuAUyVkQSqgFLZVCEvEmSIJ

Alright, that's fine. Thanks for the investigation, Sean 😃

facaiy · 2019-09-18T01:01:12Z

@seanpmorgan Happy to accept a PR which modifies those test cases separately though.

+1

@WindQAQ Maybe _correlation_cost.doc = op.doc works.

At least in tensorflow core repository, we always add document manually. Good question, I'll look into it if I have time :-)

@PyExtreme Welcome, PyExtreme, please address Sean's comments about docstring, thank you 😄

PyExtreme · 2019-09-18T11:49:15Z

@seanpmorgan , I will look into it surely and let you know.

facaiy

Looks good for me. I'd like to merge it if there is no opposition. Thank @PyExtreme

facaiy · 2019-09-19T05:42:06Z

@fsx950223 Hi, I filed an issue #526 for test cases of correlation cost. Let's fix them in the next PR, what do you think :-)

# Conflicts: # tensorflow_addons/layers/optical_flow_test.py

seanpmorgan · 2019-09-19T16:17:18Z

Thanks @PyExtreme and everyone else! Merging this in as its blocking a couple of things, but please review and let us know in a separate PR/issue if there are problems

* Add Docstring CorrelationCost * Add CorrelationCost Documentation * Small reformat

* Add docstring correlation cost (#514) * Add Docstring CorrelationCost * Add CorrelationCost Documentation * Small reformat * BLD: built on 2.0.0-rc1 * DOC: doc for 0.5.1 * Fix optical_flow test case (#527) * typo fix (#523) * CLN: fix file permission 755 in #527 * CLN: use public api, tf.keras

PyExtreme added 2 commits September 16, 2019 17:12

Add Docstring CorrelationCost

98edeb2

Merge branch 'master' of https://github.com/tensorflow/addons into ad…

68788a3

…d-docstring-correlation_cost

PyExtreme requested review from facaiy and seanpmorgan as code owners September 16, 2019 13:26

This comment has been minimized.

Sign in to view

googlebot added the cla: no label Sep 16, 2019

PyExtreme closed this Sep 16, 2019

This comment has been minimized.

Sign in to view

PyExtreme reopened this Sep 16, 2019

googlebot added cla: yes and removed cla: no labels Sep 16, 2019

seanpmorgan requested changes Sep 16, 2019

View reviewed changes

seanpmorgan reviewed Sep 16, 2019

View reviewed changes

WindQAQ self-requested a review September 16, 2019 16:23

Add CorrelationCost Documentation

0f52bc5

WindQAQ reviewed Sep 16, 2019

View reviewed changes

fsx950223 reviewed Sep 17, 2019

View reviewed changes

Small reformat

639178a

This comment has been minimized.

Sign in to view

googlebot added cla: no and removed cla: yes labels Sep 17, 2019

seanpmorgan added the kokoro:force-run label Sep 17, 2019

kokoro-team removed the kokoro:force-run label Sep 17, 2019

facaiy previously approved these changes Sep 19, 2019

View reviewed changes

Merge branch 'master' into pr/PyExtreme/514

4f7c24c

# Conflicts: # tensorflow_addons/layers/optical_flow_test.py

seanpmorgan dismissed facaiy’s stale review via 4f7c24c September 19, 2019 13:53

seanpmorgan added the kokoro:force-run label Sep 19, 2019

kokoro-team removed the kokoro:force-run label Sep 19, 2019

seanpmorgan approved these changes Sep 19, 2019

View reviewed changes

seanpmorgan merged commit 0e9ba61 into tensorflow:master Sep 19, 2019

facaiy pushed a commit to facaiy/addons that referenced this pull request Sep 20, 2019

Add docstring correlation cost (tensorflow#514)

cb52e05

* Add Docstring CorrelationCost * Add CorrelationCost Documentation * Small reformat

jondo mentioned this pull request Mar 31, 2022

Docs: Fix one link to the FlowNet paper #2684

Merged

2 tasks

Add docstring correlation cost #514

Add docstring correlation cost #514

Uh oh!

Conversation

PyExtreme commented Sep 16, 2019 • edited by facaiy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

PyExtreme commented Sep 16, 2019

Uh oh!

googlebot commented Sep 16, 2019

Uh oh!

seanpmorgan Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

seanpmorgan Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

PyExtreme Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

WindQAQ left a comment

Choose a reason for hiding this comment

Uh oh!

PyExtreme commented Sep 16, 2019

Uh oh!

fsx950223 commented Sep 17, 2019

Uh oh!

seanpmorgan commented Sep 17, 2019

Uh oh!

fsx950223 Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PyExtreme Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

fsx950223 commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanpmorgan commented Sep 17, 2019

Uh oh!

fsx950223 commented Sep 17, 2019

Uh oh!

seanpmorgan commented Sep 17, 2019

Uh oh!

This comment has been minimized.

seanpmorgan commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WindQAQ commented Sep 17, 2019

Uh oh!

facaiy commented Sep 18, 2019

Uh oh!

PyExtreme commented Sep 18, 2019

Uh oh!

facaiy left a comment

Choose a reason for hiding this comment

Uh oh!

facaiy commented Sep 19, 2019

Uh oh!

seanpmorgan commented Sep 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

PyExtreme commented Sep 16, 2019 •

edited by facaiy

Loading

fsx950223 Sep 17, 2019 •

edited

Loading

fsx950223 commented Sep 17, 2019 •

edited

Loading

seanpmorgan commented Sep 17, 2019 •

edited

Loading