Minor changes in fresh PRs (pep8, misprints, etc)#1369
Conversation
|
@menshikh-iv Could you please also incorporate a minor PEP8 suggestion made by @piskvorky here. Since you are making all such fixes in this PR, I though it would be a good idea to bring this change to your notice as well. :) |
piskvorky
left a comment
piskvorky
left a comment
There was a problem hiding this comment.
A few more minor suggestions.
| return 1. - float(len(set1 & set2)) / float(len(set1 | set2)) | ||
| """ | ||
| A distance metric between set representation. | ||
| Returns 1 minus the intersection divided by union. |
There was a problem hiding this comment.
PEP257: docstrings in imperative mode ("do X, return Y", not "does X, returns Y").
| """ | ||
| A distance metric between set representation. | ||
| Returns 1 minus the intersection divided by union. | ||
| Returns a value in range <0, 1> where values closer to 0 mean less distance and thus higher similarity. |
There was a problem hiding this comment.
less => smaller.
I see this is a potential gotcha for our users, because other metrics in gensim are similarity (not distance). How about we put this info directly into the method name -- jaccard_distance instead of jaccard_set?
|
@chinmayapancholi13 thank you |
|
@piskvorky Need to walk with an automatic tool throughout the code (especially the old one) and fix code style issues (it will be a very big diff) |
|
But this is very important. New users come and write code according to the code they see (this code is not perfect). |
|
Agreed! Fixing the existing code has been our priority for a long time now. Except don't use an automatic tool please (except as rough guidance). Use common sense to make the code readable and easy to understand and maintain, and at the same time catch other non-PEP8 lapses too: direct formatting of strings inside |
|
Automatic linters are the solution for code style adopted in most projects, for example see this presentation from itertools maintainer. |
|
If it works well, sure. There was one PR produced by an automatic linter recently, and that's definitely not what we want in gensim. |
|
I think It's a good idea to use linter for highlight the mistakes in codestyle, but fix it manually. Also, now flake8 break the build if some PEP8 problem founded in PR code and a user can fix them by reading the Travis log. |
4 participants
I have fixed all recent comments from you @piskvorky + some code style fixes. Now it's okay?
cc @tmylk