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 pyton wrapper for row conv operator. #7575

Merged
merged 10 commits into from
Jan 24, 2018
Merged

Conversation

pkuyym
Copy link
Contributor

@pkuyym pkuyym commented Jan 16, 2018

Resolves #7555

… fix-7555

Conflicts:
	doc/api/v2/fluid/layers.rst
	python/paddle/v2/fluid/layers/nn.py
Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

Please to add unit test.


Args:
input (Variable): Input variable, a 2D LoDTensor with shape [T, D].
future_context_size (int): Future context size.
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to give example, assumed future_context_size = 2, the context size of convolution is 3 ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

"""
helper = LayerHelper('row_conv', **locals())
dtype = helper.input_dtype()
filter_shape = [future_context_size + 1, input.shape[1]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to check the input.shape.size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the exception information is informational enough.

In the above equation:

* :math:`Out_{i}`: The i-th row of output variable with shape [1, D].
* :math:`\\tau`: Future context size. Please note, the shape of
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick clarification: The value of $\tau$ is future_context_size + 1 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

91345395-5355-49ee-aa49-ea3773afb587

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, actually i was concerned about the wording in your doc: ":math:\\tau: Future context size."

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for this minor and maybe frustrating comment, but I think if $\tau = future_context_size$, then the formula in Line: 1983 should have a $i + \tau + 1$.

Maybe I misunderstood something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is correct, we execute future context size + 1 times of sum operations.

Copy link
Contributor Author

@pkuyym pkuyym Jan 22, 2018

Choose a reason for hiding this comment

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

The range of j is [i, \tau + i]. Should be executed $i + \tau - i + 1$ == $\tau + 1$ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.. I missed that

"""Row Conv Operator. This layer will apply lookahead convolution to
**input**. The input variable should be a 2D LoDTensor with shape [T, D].
Parameters with shape [future_context_size + 1, D] will be created. The math
equation of row convolution is as following:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo: following -> follows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

sidgoyal78
sidgoyal78 previously approved these changes Jan 22, 2018
Copy link
Contributor

@sidgoyal78 sidgoyal78 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

LGTM+

qingqing01
qingqing01 previously approved these changes Jan 22, 2018
… fix-7555

Conflicts:
	python/paddle/v2/fluid/layers/nn.py
@pkuyym pkuyym dismissed stale reviews from qingqing01 and sidgoyal78 via ce87b28 January 23, 2018 03:29
… fix-7555

Conflicts:
	python/paddle/v2/fluid/layers/nn.py
	python/paddle/v2/fluid/tests/test_layers.py
@pkuyym pkuyym merged commit de89b47 into PaddlePaddle:develop Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants