Potential issue with mlp transform

[TomNong]

Hi,

Currently, there is an issue in _mlp_transform https://github.com/asyml/texar/blob/master/texar/modules/connectors/connectors.py#L73 , thanks @huzecong who found this issue.

For input with shape [batch_size, max_time, dim], current _mlp_transform reshapes it to [batch_size, max_time * dim] and processes linear layer transform. The transform matrix has shape [max_time * dim, mlp_output_dim], which is equivalent to max_time number of smaller matrixes with size [dim, mlp_output_dim]. However, regarding same-time-vector should transform with same matrix, current _mlp_transform can not make transform in that way. Should _mlp_transform be modified?

[ZhitingHu]

_mlp_transform is used in connectors. It looks all connectors using _mlp_transform actually do not consider existence of the time dimension? If so, we can probably just change the _mlp_transform docstring (i.e., restrict the usage of _mlp_transform to not considering the time dimension)

@huzecong what do you think?

[huzecong]

I think the docstring should be changed to reduce confusion. I also think we should change the implementation because it makes more sense, and it's possible that the user passes a multi-dimensional tensor (not necessarily containing a time dimension) to the connector.

[ZhitingHu]

If we don't assume the input to _mlp_transform has a time dimension, then the current implementation is ok, right?

If so, only the docstring need update. Connector is not for processing tensors with a time dimension.

[huzecong]

If all input tensors are 2D, then yes, the current impl. is fine.

[ZhitingHu]

Even if it's 3D or higher-dim, the current one should be also fine. Given input [batch_size, d1, d2, ..., dn], we do not make any assumption about time, all d1, .., dn are treated the same and transformed w/ a dense layer. There is no sharing needed for time dimension.

Making this clear in the docstring would be fine

[huzecong]

I see, now it makes sense to me. I agree that changing the docstring would suffice.

[TomNong]

Got it, I'll fix the docstring.

[ZhitingHu]

Pls fix the docstring and close the issue with a PR

[TomNong]

Got it.