Skip to content

Conversation

@sayoojbk
Copy link
Contributor

I have ported most of the code to tf 2.0 but for some reasons the tests are not running on my system.

Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Hello, @sayoojbk, thanks for the contribution! Please do some changes according to comments down below. Also, it would be great to run tests locally first especially for cpp kernel part. You know... it is hard to be a human compiler haha. Thanks!

@sayoojbk sayoojbk changed the title Connected components PR [WIP] Connected components PR Aug 21, 2019
@Squadrick Squadrick added the wip Work in-progress label Aug 22, 2019
Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

LGTM. I think we could rename connected_ops.py to either connected_components_ops.py or segmentation_ops.py, so does connected_ops_test.py.

@WindQAQ WindQAQ added kokoro:force-run and removed wip Work in-progress labels Aug 27, 2019
@sayoojbk
Copy link
Contributor Author

LGTM. I think we could rename connected_ops.py to either connected_components_ops.py or segmentation_ops.py, so does connected_ops_test.py.

Ya was about to ask you about that. In the TensorFlow contrib module this was named as segmentation_ops but since the naming convention of connnected_components made more sense so thought could name it connected_component_ops.py but there also while function calling in the connected_components_ops_test.py the calling of the function namely connected_components.connected_components seems a bit out of a good naming convention manner and connected_ops.connected_components seemed better. Just let me know whichever naming convention seems better and will soon make that changes.

@WindQAQ
Copy link
Member

WindQAQ commented Aug 27, 2019

As you say, could we call it connected_components.py? And in __init__.py, import it like

from tensorflow_addons.image.connected_components import connected_components

So in the testing file connected_components_test.py, you could import it via

from tensorflow_addons.image import connected_components

and can use it directly

connected_components(image)

@sayoojbk
Copy link
Contributor Author

sayoojbk commented Aug 27, 2019

As you say, could we call it connected_components.py? And in __init__.py, import it like

from tensorflow_addons.image.connected_components import connected_components

So in the testing file connected_components_test.py, you could import it via

from tensorflow_addons.image import connected_components

and can use it directly

connected_components(image)

Ya, but when the function has to be called for example like this:
self.assertAllEqual(self.evaluate( connected_components.connected_components(arr)), expected)
seems a bit odd. Then maybe I could just call the function in the connected_components_test.py as

from tensorflow_addons.image.connected_components import connected_components

@WindQAQ
Copy link
Member

WindQAQ commented Aug 27, 2019

Go ahead :P Don't forget to run sanity check locally

@sayoojbk
Copy link
Contributor Author

Go ahead :P Don't forget to run sanity check locally

Ya, my bad :P Will do all the checks and let you know when it's ready for merging!

@sayoojbk sayoojbk changed the title [WIP] Connected components PR Connected components PR Aug 27, 2019
@sayoojbk
Copy link
Contributor Author

sayoojbk commented Aug 27, 2019

Hey, @WindQAQ I guess I have made most of the changes suggested by you and ran all the tests and all have passed. It looks ready to be merged to the main branch

Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the contribution again!

@WindQAQ WindQAQ merged commit 2d977c8 into tensorflow:master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants