Fix `FunctionHook` documentation #5188

niboshi · 2018-08-06T11:05:38Z

Fixed some incorrect/missing information.

beam2d

Added some comments.

I also added suggestions on fixing document around this change, but if you feel it's too much for this PR, it's ok to do it in another one.

beam2d · 2018-08-07T00:59:12Z

chainer/function_hook.py

@@ -59,9 +66,10 @@ class FunctionHook(object):
        >>> with chainer.function_hooks.TimerHook() as m:
        ...   _ = model1(x)
        ...   y = model2(x)


It's not related to the current change, but could you make the above example code more readable by inserting blank lines appropriately?

Hmm, can we insert blank lines in doctest-style code samples?

Yes, if you prefix the blank lines with >>> or ... like follows.

""" >>> class Model(chainer.Chain): ... def __init__(self): ... super(Model, self).__init__() ... with self.init_scope(): ... self.l = L.Linear(10, 10) ... ... def __call__(self, x1): ... return F.exp(self.l(x1)) ... >>> model1 = Model() >>> model2 = Model() >>> >>> x = chainer.Variable(np.zeros((1, 10), np.float32)) >>> >>> with chainer.function_hooks.TimerHook() as m: ... _ = model1(x) ... y = model2(x) ... >>> model3 = Model() >>> z = model3(y) >>> >>> print('Total time : {}'.format(m.total_time())) ... # doctest:+ELLIPSIS Total time : ... """

By the way, is it necessary to write this example in doctest style?

beam2d · 2018-08-07T01:03:42Z

chainer/function_hook.py

@@ -76,15 +84,24 @@ class FunctionHook(object):
       as a thread local object. So, function hooks registered
       are different depending on threads.

+    If the hook is registered in this way, :meth:`~chainer.FunctionHook.added`


It takes time for me to notice that this sentence is a continuation of the last paragraph above the long example/note sections. How about moving it to the end of the last paragraph?

beam2d · 2018-08-07T01:06:50Z

chainer/function_hook.py

    Function hooks registered in this way can be removed by
-    :meth:`~chainer.Function.delete_hook` method.
+    :meth:`~chainer.FunctionNode.delete_hook` method.
    Contrary to former registration method, function hooks are registered
    only to the function which :meth:`~chainer.FunctionNode.add_hook`
    is called.


It's also not related to this change, but I think it's a good chance to fix some expressions:

The other one is to register directly to... -> register it directly

Contrary to former registration method, ... -> the former registration method

only to the function which add_hook is called. -> only to the function whose add_hook method is called.

beam2d · 2018-08-07T01:08:53Z

chainer/function_hook.py

@@ -76,15 +84,24 @@ class FunctionHook(object):
       as a thread local object. So, function hooks registered
       are different depending on threads.

+    If the hook is registered in this way, :meth:`~chainer.FunctionHook.added`
+    and :meth:`~chainer.FunctionHook.deleted` are given ``None`` as the
+    ``function`` argument.


How about rephrasing (matching with the below one):
None is passed to the function argument of added and deleted.

Is "pass to an argument" a correct expression? Maybe "as" is better?

-> "None is passed as the function argument of added and deleted."

Fixed using "passed as".

Ah, yes you are right. I was confused with "pass to function as an argument".

beam2d · 2018-08-07T01:10:48Z

chainer/function_hook.py

    Contrary to former registration method, function hooks are registered
    only to the function which :meth:`~chainer.FunctionNode.add_hook`
    is called.

+    If the hook is registered in this way, the :class:`~chainer.FunctionNode`
+    instance is given as the ``function`` argument of


Suggestion (similar to the above one): given as -> passed to

Fixed using "passed as".

beam2d · 2018-08-07T01:13:15Z

chainer/function_hook.py

@@ -118,7 +136,8 @@ def deleted(self, function=None):

        Args:
            function(~chainer.FunctionNode): Function object to which
-                the function hook is deleted.
+                the function hook is deleted. ``None`` if the function hook
+                had been registered globally.


I'm not confident about my expression either, but the hook is unregistered (the state of "being registered" is negative) at the moment of this callback.

According to @hvy, "had been" is okay but "was" is more natural expression. I fixed as such.

beam2d · 2018-08-07T01:13:47Z

chainer/function_hook.py

@@ -118,7 +136,8 @@ def deleted(self, function=None):

        Args:
            function(~chainer.FunctionNode): Function object to which


(again, suggestion outside of this change)
to which -> from which

hvy · 2018-08-07T10:32:05Z

chainer/function_hook.py

@@ -76,14 +84,24 @@ class FunctionHook(object):
       as a thread local object. So, function hooks registered
       are different depending on threads.

-    The other one is to register directly to
+    The other one is to register it directly to
    :class:`~chainer.FunctionNode` object with


Nitpick but
with -> with the

hvy · 2018-08-07T10:32:57Z

chainer/function_hook.py

    :class:`~chainer.FunctionNode` object with
-    :meth:`~chainer.Function.add_hook` method.
+    :meth:`~chainer.FunctionNode.add_hook` method.
    Function hooks registered in this way can be removed by


Nitpick, actually not directly related to this PR...
registered in this way ... by -> registered this way ... by the

hvy · 2018-08-07T10:34:46Z

chainer/function_hook.py

+    only to the function whose :meth:`~chainer.FunctionNode.add_hook`
+    method is called.
+
+    If the hook is registered globally using ``with`` statement, ``None`` is


I'd place an "a" before the "with". What do you think?

toslunar · 2018-09-12T13:46:23Z

Jenkins, test this please.

chainer-ci · 2018-09-12T14:31:59Z

Jenkins CI test (for commit dc0ee72, target branch master) succeeded!

toslunar

LGTM. More fixes could be done in a new PR.

Fix `FunctionHook` documentation Conflicts: chainer/function_hook.py

niboshi changed the title ~~Fix FunctionHook documentation~~ Fix FunctionHook documentation Aug 6, 2018

niboshi force-pushed the function-hook-doc branch from ff5d09a to 22cd2a5 Compare August 6, 2018 22:03

Fix FunctionHook documentation

6e0a7e2

niboshi force-pushed the function-hook-doc branch from 22cd2a5 to 6e0a7e2 Compare August 6, 2018 22:08

beam2d reviewed Aug 7, 2018

View reviewed changes

niboshi force-pushed the function-hook-doc branch from ff43b55 to 095e04b Compare August 7, 2018 04:31

Fix English expression

d0a1248

niboshi force-pushed the function-hook-doc branch from 095e04b to d0a1248 Compare August 7, 2018 04:39

niboshi added 2 commits August 7, 2018 13:47

Rearrange the sentences to pull together function argument explanation

9be6e8c

Fix docstring of added and deleted callbacks

e3e5640

hvy reviewed Aug 7, 2018

View reviewed changes

toslunar added the cat:document Documentation such as function documentations, comments and tutorials. label Aug 10, 2018

Merge branch 'master' into function-hook-doc

c7cfef9

toslunar self-assigned this Sep 12, 2018

Merge branch 'master' into function-hook-doc

dc0ee72

toslunar approved these changes Sep 13, 2018

View reviewed changes

toslunar merged commit d4f99d2 into chainer:master Sep 13, 2018

toslunar added this to the v5.0.0rc1 milestone Sep 13, 2018

toslunar added the to-be-backported Pull request that should be backported. label Sep 13, 2018

toslunar added a commit to toslunar/chainer that referenced this pull request Sep 13, 2018

Merge pull request chainer#5188 from niboshi/function-hook-doc

57eea6c

Fix `FunctionHook` documentation Conflicts: chainer/function_hook.py

toslunar mentioned this pull request Sep 13, 2018

[backport] Fix FunctionHook documentation #5339

Merged

niboshi deleted the function-hook-doc branch September 13, 2018 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `FunctionHook` documentation #5188

Fix `FunctionHook` documentation #5188

niboshi commented Aug 6, 2018

beam2d left a comment

beam2d Aug 7, 2018

niboshi Aug 7, 2018 •

edited

Loading

beam2d Aug 8, 2018 •

edited

Loading

beam2d Aug 7, 2018

niboshi Aug 7, 2018

beam2d Aug 7, 2018

niboshi Aug 7, 2018

beam2d Aug 7, 2018

niboshi Aug 7, 2018

niboshi Aug 7, 2018

beam2d Aug 8, 2018

beam2d Aug 7, 2018

niboshi Aug 7, 2018

beam2d Aug 7, 2018

niboshi Aug 7, 2018

niboshi Aug 7, 2018

beam2d Aug 7, 2018

niboshi Aug 7, 2018

hvy Aug 7, 2018

hvy Aug 7, 2018

hvy Aug 7, 2018

toslunar commented Sep 12, 2018

chainer-ci commented Sep 12, 2018

toslunar left a comment

		@@ -118,7 +136,8 @@ def deleted(self, function=None):

		Args:
		function(~chainer.FunctionNode): Function object to which

Fix FunctionHook documentation #5188

Fix FunctionHook documentation #5188

Conversation

niboshi commented Aug 6, 2018

beam2d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niboshi Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

beam2d Aug 8, 2018 • edited Loading

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

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

toslunar commented Sep 12, 2018

chainer-ci commented Sep 12, 2018

toslunar left a comment

Choose a reason for hiding this comment

Fix `FunctionHook` documentation #5188

Fix `FunctionHook` documentation #5188

niboshi Aug 7, 2018 •

edited

Loading

beam2d Aug 8, 2018 •

edited

Loading