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

Enrich ConvShift to support sequence data input #2133

Closed
wants to merge 9 commits into from

Conversation

pkuyym
Copy link
Contributor

@pkuyym pkuyym commented May 15, 2017

fixes #2132

@pkuyym pkuyym requested review from lcy-seso and luotao1 May 15, 2017 03:30
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

建议将修改逻辑放进matrix函数里面,现在修改的和原有matrix里的重复太多。代码可读性不好。

outV->circularConv(*inV0, *inV1);
} else {
circularConvSeq();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

将circularConvSeq函数挪到matrix.cpp里面,这个函数的接口应该类似circularConv。即
outV->circularConv(*inV0, *inV1, *seqstartposition);

这样200-207行,可以修改为

if (inLayer0.sequenceStartPositions !=nullptr) {
} else {
}

去掉isSeqType这个函数。

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

Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

添加单测

outG->circularConvDerivative(*outG, *inV0, *inV1, *inG0, *inG1);
} else {
CHECK(!inG0 || !inG1) << "Not supported";
circularConvSeqDerivative();
Copy link
Contributor

Choose a reason for hiding this comment

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

这里修改同forward函数

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

real* outV = out->getData();

int leftCtxLen = (width1 - 1) / 2;
for (size_t x = 0; x < numSeqs - 1; x++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for循环最后的x++统一改成++x,下同

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

int index = i + j - leftCtxLen;
index = (index + curSeqWidth) % curSeqWidth;
int outVRowOffset = i / width0;
int outVColOffset = i % width0;
Copy link
Contributor

Choose a reason for hiding this comment

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

113和114行只和i有关,应该放在109行的for循环里面,不要放在110行的for循环里面,这样会多算很多次。

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

for (size_t i = 0; i < curSeqWidth; i++) {
for (size_t j = 0; j < width1; ++j) {
int index = i + j - leftCtxLen;
index = (index + curSeqWidth) % curSeqWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

111行和112行可以写在一块。

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

CHECK_EQ(height1, inG1->getHeight());
CHECK_EQ(width1, inG1->getWidth());
CHECK_EQ(height0, outG->getHeight());
CHECK_EQ(width0, outG->getWidth());
Copy link
Contributor

Choose a reason for hiding this comment

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

cheack_eq有点多,可以只保留143,148,149三行

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个check是必要的,梯度、输入等的维度必须严格一致

real* inV0 = in0->getData();
real* inV1 = in1->getData();
real* inGV0 = inG0->getData();
real* inGV1 = inG1->getData();
Copy link
Contributor

Choose a reason for hiding this comment

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

命名的时候,GV都可以改成G,下同。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

延伸了Matrix函数的命名方式,GV代表真实的矩阵,感觉比较合适

int inGV0RowOffset = index / width0;
int inGV0ColOffset = index % width0;
int outGVRowOffset = i / width0;
int outGVColOffset = i % width0;
Copy link
Contributor

Choose a reason for hiding this comment

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

修改意见同forward

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

Copy link
Contributor

@lcy-seso lcy-seso left a comment

Choose a reason for hiding this comment

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

请先保证单测通过。

@@ -69,122 +66,12 @@ bool ConvShiftLayer::init(const LayerMap& layerMap,
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

22 ~ 42行的注释也需要对应地修改。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已经移到Matrix中

@@ -69,122 +66,12 @@ bool ConvShiftLayer::init(const LayerMap& layerMap,
return true;
}

bool ConvShiftLayer::isSeqType() {
Copy link
Contributor

Choose a reason for hiding this comment

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

请先加单测保证 test_layer_grad 一定可以通过。计算程序,硬看很难保证计算结果的正确。

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

@pkuyym
Copy link
Contributor Author

pkuyym commented May 15, 2017

@luotao1 谢谢review,卷积逻辑已merge到matrix,现在ConvShift层应该比较清爽,对sequence和non-sequence的卷积逻辑已经并在一起

@pkuyym pkuyym requested a review from qingqing01 May 17, 2017 03:27
data.value->add(-0.5);
if (testLayerName != "prelu") {
data.value->sigmoid(*data.value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

406-408行可以去掉,这里不会用prelu这个layer

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

@luotao1
Copy link
Contributor

luotao1 commented Feb 1, 2019

感谢您给PaddlePaddle贡献代码。由于Paddle V1/V2版本已不再维护,相关代码也已从develop分支上删除,因此关闭您的PR,欢迎您向Paddle最新版-Fluid贡献代码。
Thanks for contributing to PaddlePaddle! Since V1/V2 will not be maintained anymore, and related codes have been deleted from develop branch as well, we close this PR. Welcome to contribute to Fluid——the latest version of PaddlePaddle.

@luotao1 luotao1 closed this Feb 1, 2019
heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021
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.

Enrich ConvShift to support sequence data input
3 participants