Hide FunctionNode classes from `chainer.functions` namespace #4421

hvy · 2018-02-28T12:39:33Z

This PR hides all FunctionNode implementations from functions/__init__.py since these are considered implementation details and should not "be visible" to the user.

Some exceptions are FunctionNodes with states that are exposed through APIs such as the indices for the max pooling classes and the random states in dropout and gaussian noise.

Tests are modified accordingly to not rely on these classes directly. There are few exceptions including the the ones above.

Remove FunctionNode imports from functions/__init__
Do not use FunctionNodes classes directly in tests if possible
~~[ ] Remove FunctionNode's __init__'s default arguments~~ (Maybe better done in a separate PR)

Note: Reviewing commit by commit is probably much easier than looking at the total diff.

hvy · 2018-03-06T09:51:36Z

Rebased on the current master. Need to fix newly introduced FunctionNodes.

functions.activations.Softplus
functions.arrays.Permutate
functions.math.exponential_m1.Expm1

hvy · 2018-03-06T10:32:30Z

Fixed the above mentioned 3 FunctionNodes.

hvy · 2018-03-06T10:34:34Z

Ready for review, and very open for discussions.

hvy · 2018-05-28T06:35:40Z

I'll fix the broken commit history.

hvy · 2018-05-29T10:51:21Z

@kmaehashi Rebased and fixed the commit history. Could you please take at a look?

kmaehashi

Please check comments.
Could you add this to the upgrade guide?

kmaehashi · 2018-06-06T10:43:17Z

chainer/functions/__init__.py

 from chainer.functions.math.trigonometric import tan  # NOQA
-from chainer.functions.math.trigonometric import Tan  # NOQA

 from chainer.functions.noise.dropout import dropout  # NOQA
 from chainer.functions.noise.dropout import Dropout  # NOQA


Should it be removed? (Dropout, Gaussian, MaxPooling2D, MaxPoolingND are left)

Those are left on purpose, as they're stateful. For Dropout and Gaussian we sometimes want to reuse their random state/noise. As for the pooling functions, we similarly want to extract the resulting indices.

kmaehashi · 2018-06-06T10:47:27Z

chainer/functions/pooling/average_pooling_2d.py

@@ -188,7 +188,8 @@ def backward(self, indexes, grad_outputs):
 def average_pooling_2d(x, ksize, stride=None, pad=0):
    """Spatial average pooling function.

-    This function acts similarly to :class:`~functions.Convolution2D`, but
+    This function acts similarly to
+    :class:`~functions.connection.convolution_2d.Convolution2D`, but


It should point :func:~chainer.functions.convolution_2d.
Same for other pooling functions.

Thanks, fixed.

kmaehashi · 2018-06-06T10:55:45Z

chainer/functions/pooling/upsampling_2d.py

-        It is because :attr:`~chainer.functions.MaxPooling2D.indexes` is never
-        created and stored in the :attr:`~chainer.functions.MaxPooling2D`
+        It is because
+        :attr:`~chainer.functions.pooling.max_pooling_2d.MaxPooling2D.indexes`


Oh, I didn't know this fact. So users should directly work with MaxPooling2D to use F.upsampling_2d...

Yes unfortunately... PyTorch's max_pool**, takes a boolean flag by the way and returns either the output or a pair of (output, indices) depending on the flag. That'd be another way to do it.

kmaehashi · 2018-06-06T10:59:04Z

tests/chainer_tests/functions_tests/math_tests/test_logsumexp.py

@@ -246,19 +246,19 @@ def test_double_backward_negative_multi_axis_invert_gpu(self):

    def test_invalid_axis_type(self):
        with self.assertRaises(TypeError):
-            functions.Sum([0])
+            functions.logsumexp(self.x, [0])


kmaehashi · 2018-06-06T11:03:10Z

tests/chainer_tests/functions_tests/pooling_tests/test_spatial_pyramid_pooling_2d.py

@@ -190,11 +190,6 @@ def check_invalid_poolings(self):
        with self.assertRaises(ValueError):
            functions.spatial_pyramid_pooling_2d(self.v, 3, pooling='avg')

-        with testing.assert_warns(DeprecationWarning), \


I think it makes sense to leave this test.

Added the test back.

hvy · 2018-06-14T04:39:58Z

Hm that's what I thought. I don't think it makes a significant difference whether we remove it in this major release or in the next in that case. Please correct me if I am wrong.

kmaehashi · 2018-06-14T05:39:28Z

After v5 release, users of v4 may feel "F.MaxPooling2D suddenly disappeared although I was using documented API."

How about updating documents here in separate PR and then backporting it to v4?
https://github.com/chainer/chainer/pull/4421/files#r193370068
In that case, I think it's OK to remove MaxPooling2D in this PR.

hvy · 2018-06-15T01:39:08Z

Thanks for the suggestion, but this PR is not going to be backported?

kmaehashi · 2018-06-15T02:50:43Z

I mean:

Do not fix documentation of chainer/functions/pooling/upsampling_2d.py in this PR. Do not backport this PR.
Create new PR to fix the documentation (to suggest use of F.max_pooling_2d(return_indices=True). Then backport the PR.

hvy · 2018-06-18T02:27:48Z

Sorry I now see what you mean. I think that's an acceptable solution. I'll create a PR to fix the documentation once #4890 is merged.

hvy · 2018-06-23T08:14:36Z

jenkins, test this please.

hvy · 2018-06-23T09:22:08Z

jenkins, test this please.

chainer-ci · 2018-06-23T09:50:59Z

Jenkins CI test (for commit 67c49aa) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

hvy · 2018-06-23T14:08:59Z

jenkins, test this please.

chainer-ci · 2018-06-23T14:30:56Z

Jenkins CI test (for commit aeded94) succeeded without errors!

kmaehashi · 2018-06-29T08:33:49Z

@hvy Could you resolve conflict? (Sorry I had to merge this quickly...)

hvy · 2018-06-29T08:57:36Z

@kmaehashi Done!

hvy · 2018-06-29T09:23:27Z

jenkins, test this please.

kmaehashi · 2018-06-29T09:35:20Z

Thanks! Travis failure seems not related to this PR.
I'll merge this after Jenkins pass.

chainer-ci · 2018-06-29T09:50:48Z

Jenkins CI test (for commit 4caca0c) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

kmaehashi · 2018-06-29T09:57:09Z

LGTM! Test failures are not related to this PR.

hvy added the no-compat Change that disrupts backward compatibility. label Feb 28, 2018

hvy force-pushed the functionnodes-impl branch from 034d865 to adf4844 Compare March 6, 2018 09:50

hvy changed the title ~~[WIP] FunctionNodes as implementation details~~ FunctionNodes as implementation details Mar 6, 2018

hvy force-pushed the functionnodes-impl branch from 90ea527 to a211c86 Compare March 12, 2018 00:43

kmaehashi force-pushed the master branch from 4801b36 to 1759406 Compare March 13, 2018 08:01

kmaehashi self-assigned this May 28, 2018

hvy added 14 commits May 29, 2018 19:33

Hide function implementations from public API

d241438

Use full paths in functions

d600936

Use full path in rst

3842ab5

Add FunctionNodes back for those that contain public states

4aecf77

Remove FunctionNodes from tests

0ca668a

Fix softmax cross entropy tests

464d726

Fix exponential tests

890aa52

Fix hyperbolic tests

6f634a7

Fix logarithm 1p test

60a3d30

Fix trigonometric tests

5e884aa

Remove obsolete test

d143753

Fix doctest failure

416bc4f

Fix flake8 warnings

39c0b79

Hide recently introduced FunctionNode implementations

e44b539

hvy force-pushed the functionnodes-impl branch from a211c86 to e44b539 Compare May 29, 2018 10:47

Fix Im2Col

7986906

Fix Fmod

8408422

kmaehashi requested changes Jun 6, 2018

View reviewed changes

kmaehashi added this to the v5.0.0b2 milestone Jun 6, 2018

beam2d modified the milestones: v5.0.0b2, v5.0.0b3 Jun 21, 2018

hvy removed the st:blocked-by-another-pr State indicating that another ticket is preventing this ticket from being closed/merged. label Jun 23, 2018

Merge branch 'master' into functionnodes-impl

c7c13d4

Simplify spatial_pyramid_pooling_2d

67c49aa

Fix upsampling_2d test

aeded94

Merge branch 'master' into functionnodes-impl

adb2951

toslunar mentioned this pull request Jun 28, 2018

In-place update in F.batch_renormalization #5014

Merged

3 tasks

kmaehashi approved these changes Jun 29, 2018

View reviewed changes

Merge branch 'master' into functionnodes-impl

4caca0c

kmaehashi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Jun 29, 2018

kmaehashi merged commit 000181f into chainer:master Jun 29, 2018

hvy deleted the functionnodes-impl branch June 29, 2018 10:10

kmaehashi changed the title ~~FunctionNodes as implementation details~~ Hide FunctionNode classes from chainer.functions namespace Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide FunctionNode classes from `chainer.functions` namespace #4421

Hide FunctionNode classes from `chainer.functions` namespace #4421

hvy commented Feb 28, 2018 •

edited

hvy commented Mar 6, 2018 •

edited

hvy commented Mar 6, 2018

hvy commented Mar 6, 2018 •

edited

hvy commented May 28, 2018

hvy commented May 29, 2018

kmaehashi left a comment

kmaehashi Jun 6, 2018

hvy Jun 7, 2018

kmaehashi Jun 6, 2018

hvy Jun 7, 2018

kmaehashi Jun 6, 2018

hvy Jun 7, 2018

kmaehashi Jun 6, 2018

kmaehashi Jun 6, 2018

hvy Jun 7, 2018

hvy commented Jun 14, 2018

kmaehashi commented Jun 14, 2018 •

edited

hvy commented Jun 15, 2018

kmaehashi commented Jun 15, 2018

hvy commented Jun 18, 2018

hvy commented Jun 23, 2018

hvy commented Jun 23, 2018

chainer-ci commented Jun 23, 2018

hvy commented Jun 23, 2018

chainer-ci commented Jun 23, 2018

kmaehashi commented Jun 29, 2018

hvy commented Jun 29, 2018

hvy commented Jun 29, 2018

kmaehashi commented Jun 29, 2018

chainer-ci commented Jun 29, 2018

kmaehashi commented Jun 29, 2018

Hide FunctionNode classes from chainer.functions namespace #4421

Hide FunctionNode classes from chainer.functions namespace #4421

Conversation

hvy commented Feb 28, 2018 • edited

hvy commented Mar 6, 2018 • edited

hvy commented Mar 6, 2018

hvy commented Mar 6, 2018 • edited

hvy commented May 28, 2018

hvy commented May 29, 2018

kmaehashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvy commented Jun 14, 2018

kmaehashi commented Jun 14, 2018 • edited

hvy commented Jun 15, 2018

kmaehashi commented Jun 15, 2018

hvy commented Jun 18, 2018

hvy commented Jun 23, 2018

hvy commented Jun 23, 2018

chainer-ci commented Jun 23, 2018

hvy commented Jun 23, 2018

chainer-ci commented Jun 23, 2018

kmaehashi commented Jun 29, 2018

hvy commented Jun 29, 2018

hvy commented Jun 29, 2018

kmaehashi commented Jun 29, 2018

chainer-ci commented Jun 29, 2018

kmaehashi commented Jun 29, 2018

Hide FunctionNode classes from `chainer.functions` namespace #4421

Hide FunctionNode classes from `chainer.functions` namespace #4421

hvy commented Feb 28, 2018 •

edited

hvy commented Mar 6, 2018 •

edited

hvy commented Mar 6, 2018 •

edited

kmaehashi commented Jun 14, 2018 •

edited