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

Add 3D operations for layers: conv3d, avg_pool3d and max_pool3d #9477

Closed
wants to merge 11 commits into from
Closed

Conversation

Kongsea
Copy link
Contributor

@Kongsea Kongsea commented Apr 27, 2017

1.Fix a TODO(jbms):

change 'rate' parameter to 'dilation_rate' for consistency with underlying op.

2.Add 3D operations for layers:

conv3d
avg_pool3d
max_pool3d

3.Replace a deprecated function in layers_test.py.

4.Add unit test for avg_pool3d and max_pool3d.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

Kongsea and others added 9 commits April 27, 2017 14:01
The function loss_ops.get_regularization_losses() from tensorflow.contrib.losses.python.losses import loss_ops has been deprecated.
Use tf.losses.get_regularization_losses() to replace it.
Update: using from tensorflow.losses import get_regularization_losses instead of tf.losses.get_regularization_losses
Use this import method: from tensorflow.python.ops.losses.losses import get_regularization_losses
@martinwicke martinwicke added the awaiting review Pull request awaiting review label Apr 27, 2017
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

The code itself is fine, but I am not sure we should add new layers to tf.contrib.layers: at this point you should use tf.layers if possible, especially since all the layers you are adding are already present in tf.layers.

Deferring to @martinwicke for a decision on whether we should add new functionality to tf.contrib.layers.

@add_arg_scope
def convolution(inputs,
num_outputs,
kernel_size,
stride=1,
padding='SAME',
data_format=None,
rate=1,
dilation_rate=1,
Copy link
Member

Choose a reason for hiding this comment

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

This name change would not be backwards compatible. If changing the API, it should be done in a backwards compatible way (albeit maybe not in contrib... @martinwicke: what's the policy for breaking changes in contrib?).

@martinwicke
Copy link
Member

It's fine to break things in contrib. The other point stands though -- ideally we'd shrink the amount of code in contrib/layers. I suppose you require the contrib versions for something like arg_scope?

@Kongsea
Copy link
Contributor Author

Kongsea commented May 3, 2017

@fchollet Many researchers who write programs using tensorflow slim need the conv3d and conv3d_transpose in tf.contrib.layers, such as this issue.

@fchollet
Copy link
Member

fchollet commented May 3, 2017

@martinwicke: it is theoretically fine to break things in contrib --but is this PR worth breaking every Slim model that uses dilation? Neither Slim nor filter dilation are particularly mainstream, but it's still a number of breakages to deal with --I count ~20 in g3.

Many researchers who write programs using tensorflow slim need the conv3d and conv3d_transpose in tf.contrib.layers

For some definition of "many" --only 200k or so people have touched TF, far fewer use 3d convolutions (500-1000?), fewer use transposed 3D convolutions (100-200?), and far fewer use Slim. If we total ~5-10 people that's already a stretch.

@Kongsea
Copy link
Contributor Author

Kongsea commented May 4, 2017

@fchollet Maybe it's some users who want to use a 3D convolutions of Slim, especially in the medical field, since in medical field we need to process a series of CT slices which are organized in a 3D volume. Of course, this is from my own using experience. But I don't think this is a niche needs. So please rethink it.

@xcyan
Copy link

xcyan commented May 8, 2017

@fchollet @martinwicke Thank you for your comments! However, the comment seems not fair.

For some definition of "many" --only 200k or so people have touched TF, far fewer use 3d convolutions (500-1000?), fewer use transposed 3D convolutions (100-200?), and far fewer use Slim. If we total ~5-10 people that's already a stretch.

In fact, conv3d/conv3d_transpose has become really popular these days: please check out the latest CVPR/NIPS/ICML papers that use 3d convolution:
[1] Unsupervised Learning of 3D Structure from Images In NIPS 16 from Deepmind
[2] Learning a Probabilistic Latent Space of Object Shapes via 3D Generative-Adversarial Modeling In NIPS 2016 from Facebook & MIT
[3] Perspective Transformer Nets: Learning Single-View 3D Object Reconstruction without 3D Supervision In NIPS 2016 from UM, Adobe & Google Brain
[4] Shape Completion using 3D-Encoder-Predictor CNNs and Shape Synthesis In CVPR 2017 from Stanford
[5] Learning a Predictable and Generative Vector Representation for Objects In ECCV 2016 from CMU & Google Research
and so on.

We really appreciate if the conv3d/conv3d_transpose can be added to slim, which may benefit people working on 3D generation.
As far as I know, Torch has provided the conv3d/conv3d_transpose and that is one reason some researchers tend to avoid using TF for 3D generation.

@Kongsea
Is it possible to make any modification on your side? As far as I can see, it will cause TF developers to do some extra work (~20 breakages according to @fchollet ) which might not be the high priority for them. Thank you very much!

@martinwicke
Copy link
Member

I'm fine with this change. I think the only remaining question is whether we want to break existing models due to the rate to dilation_rate name change. @sguada, do you have a preference on this? We're definitely allowed to do it, but that doesn't necessarily mean we should.

@Kongsea
Copy link
Contributor Author

Kongsea commented May 9, 2017

OK, now for simplicity I closed this PR and opened a new PR to add the 3D operations only and leave the 'rate' parameter as original.

@Kongsea Kongsea closed this May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review Pull request awaiting review cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants