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

Add a comment that explains why the Classifier chain clears Variable attribues. #5069

Merged

Conversation

grafi-tt
Copy link
Contributor

@grafi-tt grafi-tt commented Jul 7, 2018

This PR makes the Classifier chain better explains the importance to clear the Variable attirubtes. It prevents sloppy users defines their own chain that consumes unnecessary memory.

The docstring of chainer.links.model.Classifier says:

This is an example of chain that wraps another chain. It computes the
loss and accuracy based on a given input/label pair.

So it is reasnoable to assume that users often view and modify the source to define a custom chain. In fact, such a double memory allocation bug has actually happend in a project I'm work for.

@mitmul
Copy link
Member

mitmul commented Jul 10, 2018

Thank you for the PR! How about putting the explanation into the docstring of the class Classifier? If you put the explanation to the docstring, both users who are modifying this class and those who will modify the code can see the explanation. So I think it's good to have it in the documentation to make it visible to all.

@mitmul mitmul self-assigned this Jul 10, 2018
@kmaehashi kmaehashi added the cat:document Documentation such as function documentations, comments and tutorials. label Jul 10, 2018
@grafi-tt grafi-tt force-pushed the add-comment-about-classifier-variable-clearing branch from 5904c33 to ea18ad2 Compare July 25, 2018 07:37
@grafi-tt
Copy link
Contributor Author

@mitmul Sorry for late reply. I moved the comment to a note on docstrings (with change of wordings).

Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

LGTM

@mitmul
Copy link
Member

mitmul commented Sep 10, 2018

Welcome back! Jenkins, test this please

@chainer-ci
Copy link
Member

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

@toslunar
Copy link
Member

Jenkins, test this please.

@chainer-ci
Copy link
Member

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

@mitmul
Copy link
Member

mitmul commented Sep 16, 2018

Jenkins, test this please

@chainer-ci
Copy link
Member

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

@mitmul
Copy link
Member

mitmul commented Sep 16, 2018

LGTM

@mitmul mitmul merged commit 2a87576 into chainer:master Sep 16, 2018
@kmaehashi kmaehashi added this to the v5.0.0rc1 milestone Sep 27, 2018
@grafi-tt grafi-tt deleted the add-comment-about-classifier-variable-clearing branch October 29, 2019 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:document Documentation such as function documentations, comments and tutorials.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants