DeepInsight-PCALab / CompactBilinearPooling-Pytorch

A Pytorch Implementation for Compact Bilinear Pooling.
182 stars 41 forks source link

Complex product should be used in eltwise product #3

Closed vadimkantorov closed 6 years ago

vadimkantorov commented 6 years ago

I was investigating differences in results of this package and of the original Caffe version. Things found so far:

  1. Rfft can be used directly, it is provided by pytorch_fft
  2. Complex product should be used here: https://github.com/DeepInsight-PCALab/CompactBilinearPooling-Pytorch/blob/master/CompactBilinearPooling.py#L91, the tf.multiply does complex product, original caffe version does complex product
  3. Output should be mutliplied by output_dim in order to achieve full equivalence with original caffe version
dichen-cd commented 6 years ago

Many thanks for your advice! I will investigate into that later!

dichen-cd commented 6 years ago

After some doc rushing I think tf.multiply does element-wise product instead of complex product. The comment at L141 also indicates that. And if this is true, element-wise on the real part and imag part respectively should be equivalent to element-wise product at the two parts at once.

vadimkantorov commented 6 years ago

from my tests, it's the complex product that matches the original Caffe version

very roughly based on your code, i made a version that uses new PyTorch fft support: https://gist.github.com/vadimkantorov/d9b56f9b85f1f4ce59ffecf893a1581a

dichen-cd commented 6 years ago

Awesome!

I'll check the caffe version later.

vadimkantorov commented 6 years ago

a link to original author's caffe impl (complex product): https://github.com/gy20073/compact_bilinear_pooling/blob/master/caffe-20160312/src/caffe/layers/compact_bilinear_layer.cpp#L219

if tf.multiply doesn't do complex product, it may be a bug in the tf reimpl

dichen-cd commented 6 years ago

Checked the original paper again. Although the paper doesn't make an explicit statement, I believe u r right. It makes more sense for element-wise complex product than naive production.

Many thanks for your help! I'll update this repo soon.