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

New style square function #3578

Merged
merged 2 commits into from Oct 25, 2017
Merged

New style square function #3578

merged 2 commits into from Oct 25, 2017

Conversation

kmaehashi
Copy link
Member

This PR implements new style square function.

I also fixed unary_math_function_test decorator to support testing FunctionNode.
I think this is transitional; once #3499 got merged, it is better to rewrite tests with the new function test template.

@kmaehashi kmaehashi added the cat:feature Implementation that introduces new interfaces. label Oct 17, 2017
@takagi takagi self-assigned this Oct 17, 2017
@takagi
Copy link
Member

takagi commented Oct 17, 2017

I got the situation. We don't need to keep unary_math_function_test to accept Function class once we will have moved to new style functions.

Actually the number of places where the decorator is now used is only five, so would you make the decorator accept only FunctionNode class with having some working around just to keep CI pass, for example, temporarily leaving old unary_math_function_test for Function class with renamed and removing it after we have finished to move to new style functions.

I suppose it would be enough.

@takagi
Copy link
Member

takagi commented Oct 18, 2017

We should follow API compat policy for unary_math_function_test, so my previous comment does not suit.

@kmaehashi
Copy link
Member Author

Ok, thank you for confirmation!

@niboshi niboshi added the to-be-backported Pull request that should be backported. label Oct 18, 2017
@takagi
Copy link
Member

takagi commented Oct 18, 2017

It would be more natural for Chainer to let unary_math_function_test take a Python function instead of a FunctionNode object as @beam2d mentioned on slack.

master...takagi:fix-test-math-function

Here unary_math_function_test accept a Function object just for backward compatibility and we would be able to deprecate it in the future major bump. (I will update the docstring shortly)

How about this?

@kmaehashi
Copy link
Member Author

Thank you for the suggestion, it looks good to me.

@takagi
Copy link
Member

takagi commented Oct 19, 2017

I posted a PR to fix unary_math_function_test to support new style functions as #3603. Would you rebase your change on top of it after it will have been merged into the master? Before that, you can leave this PR.

@takagi takagi added the st:blocked-by-another-pr State indicating that another ticket is preventing this ticket from being closed/merged. label Oct 19, 2017
@kmaehashi kmaehashi removed the st:blocked-by-another-pr State indicating that another ticket is preventing this ticket from being closed/merged. label Oct 24, 2017
@kmaehashi
Copy link
Member Author

Rebased.

@takagi
Copy link
Member

takagi commented Oct 25, 2017

LGTM!

@takagi takagi added this to the v4.0.0b1 milestone Oct 25, 2017
@takagi takagi merged commit ed667d5 into chainer:master Oct 25, 2017
takagi added a commit to takagi/chainer that referenced this pull request Oct 25, 2017
@kmaehashi kmaehashi deleted the new-style-square branch October 25, 2017 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature Implementation that introduces new interfaces. to-be-backported Pull request that should be backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants