Skip to content

Add "axis" and "axis_w" arguments in FC to support customized axix to reduce dim.#12971

Closed
gujinghui wants to merge 1 commit intopytorch:masterfrom
gujinghui:support_axis_w_in_FC
Closed

Add "axis" and "axis_w" arguments in FC to support customized axix to reduce dim.#12971
gujinghui wants to merge 1 commit intopytorch:masterfrom
gujinghui:support_axis_w_in_FC

Conversation

@gujinghui
Copy link
Collaborator

@gujinghui gujinghui commented Oct 23, 2018 •
edited
Loading

Add "axis" and "axis_w" arguments in FC to support customized axix to reduce dim.

@gujinghui gujinghui mentioned this pull request Oct 23, 2018
Closed
@gujinghui
Copy link
Collaborator Author

gujinghui commented Oct 23, 2018

@pytorchbot retest this please

yinghai
yinghai reviewed Oct 23, 2018
View reviewed changes
Copy link
Contributor

@yinghai yinghai left a comment

Choose a reason for hiding this comment

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

The title of the PR is misleading. For general FC op, we already support it.

This is about Nomnigraph transformation and support in IDEEP, right?

@gujinghui
Copy link
Collaborator Author

gujinghui commented Oct 24, 2018

@yinghai
No. Previously, we only support fixed axis to reduce the dims in FC, for example, 4D (nchw) to 2D (nC).
Now, we can support mutable axis in FC.

Yes, the title is misleading. Will change it.

@gujinghui gujinghui changed the title support "axis" and "axis_w" in FC Add "axis" and "axis_w" arguments in FC to support customized axix to reduce dim. Oct 24, 2018
@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch from 6373512 to b975ae3 Compare October 30, 2018 01:41
@gujinghui
Copy link
Collaborator Author

gujinghui commented Oct 30, 2018

hi @yinghai ,

Regardless of #12970 and #9488, can we merge this PR?
This one is not only for pre-converting filters.
To support customized axis in FC, we need this PR.

facebook-github-bot
facebook-github-bot reviewed Oct 30, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yinghai
yinghai suggested changes Oct 31, 2018
View reviewed changes
@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch from b975ae3 to 334dac0 Compare November 1, 2018 01:58
@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 1, 2018

@yinghai
pls help review.

@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 2, 2018

@yinghai
the code style has been corrected.
pls help merge it.
Thanks.

facebook-github-bot
facebook-github-bot reviewed Nov 7, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yinghai
yinghai suggested changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@yinghai yinghai left a comment

Choose a reason for hiding this comment

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

I think this needs rebase.

@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch from 334dac0 to 13f52f7 Compare November 10, 2018 16:37
@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 10, 2018

@yinghai
rebased. pls have a try.

@yinghai
Copy link
Contributor

yinghai commented Nov 10, 2018

Could you fix the clang format too? I found that your style seems to be consistently different from ours. :0

@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch from 13f52f7 to 55b7b1f Compare November 11, 2018 11:34
@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 11, 2018

Could you fix the clang format too? I found that your style seems to be consistently different from ours. :0

Sure. Done. Pls help review.
BTW, the format file is <root_path>/caffe2/.clang-format, other than <root_path>/.clang-format, right?

@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch 2 times, most recently from 75311c4 to bb50cdc Compare November 15, 2018 07:36
@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 15, 2018

rebased to latest.
@yinghai
please help merge it.

facebook-github-bot
facebook-github-bot reviewed Nov 15, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yinghai
Copy link
Contributor

yinghai commented Nov 15, 2018

@gujinghui Do we have any tests for this change?

@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch from bb50cdc to 5f5ed84 Compare November 16, 2018 11:08
@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 16, 2018

@yinghai
yes. We have test cases for this in caffe2/python/ideep/fc_op_test.py

rebased to latest.

facebook-github-bot
facebook-github-bot reviewed Nov 16, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yinghai
yinghai suggested changes Nov 18, 2018
View reviewed changes
@gujinghui
support "axis" and "axis_w" in FC
8a95554 
Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>
@gujinghui gujinghui force-pushed the support_axis_w_in_FC branch from 5f5ed84 to 8a95554 Compare November 19, 2018 13:08
@gujinghui
Copy link
Collaborator Author

gujinghui commented Nov 19, 2018

@yinghai
Fixed. Pls help review.

facebook-github-bot
facebook-github-bot reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@yinghai yinghai yinghai requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gujinghui @yinghai @facebook-github-bot @ezyang