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

[Lint] Pylint #330

Merged
merged 18 commits into from
Jan 1, 2019
Merged

[Lint] Pylint #330

merged 18 commits into from
Jan 1, 2019

Conversation

jermainewang
Copy link
Member

Description

This PR enables pylint check for our python codes. Related to #290 .

Checklist

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (such as [Model], [Doc], [Feature]])
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the my best knowledge, examples are either not affected by this change,
    or have been fixed to be compatible with this change
  • Related issue is referred in this PR

Changes

  • Pylint for most of the python codes under python/dgl.
  • Fix some bugs in inplace write. @ylfdq1118
  • Simplify the logic in frame with slice index type. This could be done because utils.Index now supports slice data. @BarclayII is there any model using slice on the frame?

Following codes are not enabled for lint check.

  • dgl.backend : TBD.
  • dgl.contrib : @zheng-da could you help?
  • dgl.data : need help from the owner.
  • dgl.nn : this is likely to be changed in the future.

One could check the lint results offline by (requires cpplint and pylint):

bash tests/scripts/task_lint.sh

We also need to put this in a developer guide in our doc.

@jermainewang
Copy link
Member Author

@ylfdq1118 and @BarclayII could you have a look at the changes in frame.py? Other changes are mainly style fixes.

Copy link
Collaborator

@lingfanyu lingfanyu left a comment

Choose a reason for hiding this comment

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

The changes look good.

python/dgl/batched_graph.py Outdated Show resolved Hide resolved
python/dgl/function/base.py Outdated Show resolved Hide resolved
slc = idx.slice_data()
part1 = F.narrow_row(self.data, 0, slc.start)
part2 = feats
part3 = F.narrow_row(self.data, slc.stop, len(self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this works for mxnet...

For mxnet, F.narrow_row is

def narrow_row(data, start, stop):
    return nd.slice(data, begin=start, end=stop)

If start = stop = 0, it would return the whole ndarray instead of an empty one. And if start = stop = len(self), it would throw out exception.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I noticed that as well. In fact this problem exists in the old code as well. I fixed it on my unified test branch but maybe you can fix that as well and I merge later?

Copy link
Member Author

Choose a reason for hiding this comment

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

@BarclayII I will leave it to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, is this a problem of MX or ours?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My feeling is this is a problem with MX. And actually, MXNet's behavior looks weird to me. For nd.slice, as long as begin=end, nd.slice will return a tensor with the same shape of the original tensor.

For example, if we have a=nd.normal(shape=(10, 5)), then nd.slice(a, begin=0, end=0) will return a, but nd.slice(a, begin=2, end=2) will start slicing from the 2nd row and takes out 10 rows, which goes out boundary without any errors. So the last two rows of return tensor has random values... And if argument begin goes out of range, e.g. begin=10, it throws out exception.

Copy link
Collaborator

@BarclayII BarclayII Jan 1, 2019

Choose a reason for hiding this comment

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

But if we use data[start:stop] instead of nd.slice then the begin == end case works correctly though. Also fixed that in my branch.
@zheng-da

Copy link
Member Author

Choose a reason for hiding this comment

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

Raise an issue in mx? @zheng-da please help in tracking it.

@jermainewang jermainewang merged commit 4bd4d6e into dmlc:master Jan 1, 2019
@jermainewang jermainewang deleted the pylint branch January 1, 2019 20:15
@jermainewang jermainewang mentioned this pull request Feb 18, 2019
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants