Refactor by pep8 #1550

zsef123 · 2017-08-27T04:31:16Z

First checkbox

PEP8 (without a hard 80-char constraint for long lines)

Most changes are whitespace adjustment and import order changes.

Then unused var and header are removed except in try catch, named _ var and side effects call.
Excepted case add comments 'noqa'

And test_atmodel.py, test_ldamallet_wrapper.py, test_ldamodel codes don't use fname like under code

fname = testfile()
model = self.model
model.save(testfile(), sep_limit=0)

save or load argument changed from testfile() to fname

piskvorky

Welcome changes overall, thanks! Minor fixes required, see comments.

piskvorky · 2017-08-27T07:52:25Z

gensim/corpora/__init__.py

-from .textcorpus import TextCorpus
-from .ucicorpus import UciCorpus
-from .malletcorpus import MalletCorpus
+from .mmcorpus import MmCorpus  # noqa:F401


-1: we don't want to litter the code with some tool-specific constructs.

Instead, open an issue with the tool that requires this, it's a bug on their side if they cannot handle this line.

If you find such comments in other parts of gensim, please remove them too.

@piskvorky it's not a tool bug, it's really "unused import" by PEP

But it is used -- it makes the classes available directly from the package. Without it, you won't be able to from corpora import MmCorpus.

Or how can this be done in other way, that doesn't trigger F401?

it makes the classes available directly from the package

You are right, but now in the code, it's unused import.

I know another way - use this imports (like your example) in gensim internally, but I don't think that this is a good idea. For my opinion, noqa suppress is OK.

piskvorky · 2017-08-27T07:54:17Z

gensim/examples/dmlcz/sources.py

    def idFromDir(self, path):
        assert len(path) > len(self.baseDir)
        dmlczId = open(os.path.join(path, 'dspace_id')).read().strip()
-        pathId = path[len(self.baseDir) + 1 : ]
+        pathId = path[len(self.baseDir) + 1:]


This is against PEP8 (see https://www.python.org/dev/peps/pep-0008/#other-recommendations ).

piskvorky · 2017-08-27T07:55:03Z

gensim/interfaces.py

@@ -267,7 +263,7 @@ def __iter__(self):
                # (unlike numpy). so, clip the end of the chunk explicitly to make
                # scipy.sparse happy
                chunk_end = min(self.index.shape[0], chunk_start + self.chunksize)
-                chunk = self.index[chunk_start : chunk_end]
+                chunk = self.index[chunk_start: chunk_end]


PEP8: https://www.python.org/dev/peps/pep-0008/#other-recommendations (here and elsewhere).

zsef123 · 2017-08-28T19:21:19Z

E722 | do not use bare except, specify exception instead

For example

try:
    "do something"
except:
    pass

E722 error occurs except line

In gensim code, some line add Exception class but the others not specify exception

So if not used exception class in exception scope, I just add Exception like under code

try:
    "do something"
except Exception:
    pass

zsef123 · 2017-08-28T19:59:36Z

I miss check in corpora dir files.

I'm not touch about noqa, line slice whitespace yet.

and naming like 'l' changed to suitable name

piskvorky · 2017-08-29T05:22:26Z

gensim/matutils.py

@@ -150,7 +150,7 @@ def zeros_aligned(shape, dtype, order='C', align=128):
    nbytes = np.prod(shape, dtype=np.int64) * np.dtype(dtype).itemsize
    buffer = np.zeros(nbytes + align, dtype=np.uint8)  # problematic on win64 ("maximum allowed dimension exceeded")
    start_index = -buffer.ctypes.data % align
-    return buffer[start_index: start_index + nbytes].view(dtype).reshape(shape, order=order)
+    return buffer[start_index:start_index + nbytes].view(dtype).reshape(shape, order=order)


Does not conform to PEP8 (need spaces around : here, see #1521 (comment)).

@piskvorky Sorry, Line slice white space was not my intention. I missed these line in this commit.

Anyway, I agree to input space around :.

but if input space like return buffer[start_index : start_index + nbytes], E203 error occurs. (pycodestyle issue)

Is the above line okay if an error occurs?

I don't think there's any error in the code, the version with spaces is correct. The version without spaces is incorrect. It appears to be a bug in the style-checking software. CC @menshikh-iv

zsef123 · 2017-08-31T22:10:37Z

Issue #1551

By adding # flake8: noqa to the file (docs:ignoring-entire-file)

I have adding above comment ignore file

It may be dangerous but I think __init__.py files short and simple enough so I think safely ignore flake8 errors

Excepts models/_init.py, this file contains short class.
Still, I think it's okay to ignore the error because the file is still short, and support by PEP8.

piskvorky · 2017-09-01T05:14:43Z

gensim/corpora/__init__.py

@@ -1,17 +1,17 @@
 """
 This package contains implementations of various streaming corpus I/O format.
 """
-
+# flake8: noqa


This seems to combine the worst of both worlds. Still extra comments, AND no style checking.

I think I prefer even the noqa:F401 version.

zsef123 · 2017-09-01T07:32:26Z

@piskvorky I canceled commit about ignore whole file.

F403 : from module import * used; unable to detect undefined names

And just from module import * disassembled to each functions

zsef123 · 2017-09-05T08:27:15Z

Sorry, I clicked delete accidently.

And I think I've finished applying PEP8.

white space adjustment
delete unused var( except naming _ ), unused import modules
Add specific exceptions( except Exception: )
noqa comments about flake8 F401 error ( Issue flake8 F401 error in __init__.py #1551 )
disassembled import * about flake8 F403 error

menshikh-iv · 2017-09-05T11:26:13Z

Hi @zsef123, please remove file gensim/scripts/make_wiki_lemma.py from your PR ASAP (because you broke current symlink -> we can't build package)

menshikh-iv · 2017-09-05T11:46:07Z

@zsef123 I means to remove this file from your PR (not from the repo), you need to checkout this file to develop state.

zsef123 · 2017-09-05T11:48:42Z

@menshikh-iv Sorry I check again.
But PEP8 E203 error occurs in travis

gensim/examples/dmlcz/sources.py:125:41: E203 whitespace before ':'
        intId = path[1 + path.rfind('#') :]

gensim/models/word2vec.py:1552:37: E203 whitespace before ':'
                        yield line[i : i + self.max_sentence_length]

Issue #1521

Second, line changed to
return buffer[start_index:start_index + nbytes].view(dtype).reshape(shape, order=order)
these line not occurs error in pep8 library and fine too

And

piskvorky commented 8 days ago • edited
The second option does not conform to PEP8 (see the "No:" section in https://www.python.org/dev/peps/pep-0008/#pet-peeves).Basically, the : operator has a lower priority than e.g. +, and so shouldn't have less whitespace because that implies more priority in Python, by convention.

So I input space around :. Is it problem?

menshikh-iv · 2017-09-05T11:54:29Z

@zsef123 space before : is problem here.

zsef123 · 2017-09-05T12:04:20Z

@menshikh-iv In matutils.py, I already remove space before : ( in first commits )
And I requested like under code

-    return buffer[start_index: start_index + nbytes].view(dtype).reshape(shape, order=order)
+    return buffer[start_index:start_index + nbytes].view(dtype).reshape(shape, order=order)

zsef123 7 days ago • edited Contributor
Sorry, Line slice white space was not my intention. I missed these line in this commit.
Anyway, I agree to input space around :.
but if input space like return buffer[start_index : start_index + nbytes], E203 error occurs. (pycodestyle issue)
Is the above line okay if an error occurs?

piskvorky 7 days ago • edited Owner
I don't think there's any error in the code, the version with spaces is correct. The version without spaces is incorrect. It appears to be a bug in the style-checking software. CC @menshikh-iv

So I changed to input space around :

zsef123 · 2017-09-05T12:56:34Z

@menshikh-iv Should I remove space before :? like line[i: i + self.max_sentence_length]
This line passed Travis

menshikh-iv · 2017-09-05T12:57:45Z

@zsef123 yep

menshikh-iv · 2017-09-05T14:57:42Z

Very large and useful work, thank you very much @zsef123 🔥

…orky#1550) * gensim dir PEP8 fixes * corpora dir PEP8 fixes * example dir PEP8 fixes * model/wrapper dir PEP8 fixes * models dir PEP8 fixes * parsing dir PEP8 fixes * scripts dir PEP8 fixes * similarities dir PEP8 fixes * summarization and topic_coherence dir PEP8 fixes * test dir PEP8 fixes * PEP8 E722 error fixes * PEP8 fixes * list slice whitespace PEP8 fixes * disassemble import * * Fix symlink * fix symlink * fix make_wiki_lemma file * Replace relative import to absolute * fix typo * fix E203 error

zsef123 added 10 commits August 27, 2017 07:43

gensim dir PEP8 fixes

2f09911

corpora dir PEP8 fixes

84c5ad7

example dir PEP8 fixes

5b10cbc

model/wrapper dir PEP8 fixes

0a957d4

models dir PEP8 fixes

eae37d7

parsing dir PEP8 fixes

35453c0

scripts dir PEP8 fixes

f5b52ad

similarities dir PEP8 fixes

f955339

summarization and topic_coherence dir PEP8 fixes

3215493

test dir PEP8 fixes

4bdbf3e

piskvorky requested changes Aug 27, 2017

View reviewed changes

This was referenced Aug 28, 2017

Refactor current gensim code by PEP8 #1521

Closed

flake8 F401 error in __init__.py #1551

Closed

PEP8 E722 error fixes

03735bc

PEP8 fixes

fa7d8c9

piskvorky requested changes Aug 29, 2017

View reviewed changes

list slice whitespace PEP8 fixes

624c272

piskvorky reviewed Sep 1, 2017

View reviewed changes

disassemble import *

22816c0

zsef123 force-pushed the refactor-by-pep8 branch from 7a5dd56 to 22816c0 Compare September 1, 2017 07:27

zsef123 closed this Sep 5, 2017

zsef123 deleted the refactor-by-pep8 branch September 5, 2017 07:21

zsef123 restored the refactor-by-pep8 branch September 5, 2017 08:19

zsef123 reopened this Sep 5, 2017

Fix symlink

e719cae

menshikh-iv added 2 commits September 5, 2017 16:09

Merge branch 'develop' into refactor-by-pep8

537c789

fix symlink

7bbfc22

fix make_wiki_lemma file

8240c7d

zsef123 force-pushed the refactor-by-pep8 branch from b0f77a8 to 8240c7d Compare September 5, 2017 12:05

menshikh-iv added 2 commits September 5, 2017 17:19

Replace relative import to absolute

23c4526

fix typo

8c96365

fix E203 error

f74c568

menshikh-iv merged commit 6d6f5dc into piskvorky:develop Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor by pep8 #1550

Refactor by pep8 #1550

zsef123 commented Aug 27, 2017

piskvorky left a comment

piskvorky Aug 27, 2017 •

edited

Loading

menshikh-iv Aug 28, 2017

piskvorky Aug 28, 2017

menshikh-iv Aug 28, 2017 •

edited

Loading

piskvorky Aug 27, 2017

piskvorky Aug 27, 2017

zsef123 commented Aug 28, 2017 •

edited

Loading

zsef123 commented Aug 28, 2017

piskvorky Aug 29, 2017

zsef123 Aug 29, 2017 •

edited

Loading

piskvorky Aug 29, 2017 •

edited

Loading

zsef123 commented Aug 31, 2017 •

edited

Loading

piskvorky Sep 1, 2017

zsef123 commented Sep 1, 2017 •

edited

Loading

zsef123 commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

zsef123 commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

zsef123 commented Sep 5, 2017 •

edited

Loading

zsef123 commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

Refactor by pep8 #1550

Refactor by pep8 #1550

Conversation

zsef123 commented Aug 27, 2017

piskvorky left a comment

Choose a reason for hiding this comment

piskvorky Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

menshikh-iv Aug 28, 2017

Choose a reason for hiding this comment

piskvorky Aug 28, 2017

Choose a reason for hiding this comment

menshikh-iv Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

piskvorky Aug 27, 2017

Choose a reason for hiding this comment

piskvorky Aug 27, 2017

Choose a reason for hiding this comment

zsef123 commented Aug 28, 2017 • edited Loading

zsef123 commented Aug 28, 2017

piskvorky Aug 29, 2017

Choose a reason for hiding this comment

zsef123 Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

piskvorky Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

zsef123 commented Aug 31, 2017 • edited Loading

piskvorky Sep 1, 2017

Choose a reason for hiding this comment

zsef123 commented Sep 1, 2017 • edited Loading

zsef123 commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

zsef123 commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

zsef123 commented Sep 5, 2017 • edited Loading

zsef123 commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

menshikh-iv commented Sep 5, 2017

piskvorky Aug 27, 2017 •

edited

Loading

menshikh-iv Aug 28, 2017 •

edited

Loading

zsef123 commented Aug 28, 2017 •

edited

Loading

zsef123 Aug 29, 2017 •

edited

Loading

piskvorky Aug 29, 2017 •

edited

Loading

zsef123 commented Aug 31, 2017 •

edited

Loading

zsef123 commented Sep 1, 2017 •

edited

Loading

zsef123 commented Sep 5, 2017 •

edited

Loading