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

Standardize Imports I, #176 #206

Merged
merged 6 commits into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@zuphilip
Collaborator

zuphilip commented Apr 28, 2017

The import statements for the main commands ocropus-* are
standardized:

  • Put different import statements into different lines
  • Group them into (1) standard imports, (2) third party
    imports, (3) local modules
  • Avoid the "from pylab import *"
  • Use rather "import numpy as np", "import matplotlib.pyplot as plt"

Moreover, some unused import statements were deleted and a very
few nits for readibility are done.

This work is heavily based on the work by @QuLogic in the branch
https://github.com/QuLogic/ocropy/commits/standard-import
See also #176 .

All commands from the current pipeline are tested to still work
in the usual calls (especially run-test and run-rtrain). Even more, the
outputs before and afterwards is exactly the same.

zuphilip added some commits Apr 27, 2017

Update import statements in setup.py
Some imports are not anymore used because the models or other
data files are not anymore downloaded with this script. Some
uncommented parts are now deleted.
Standardize imports for ocropus-*
The import statements for the main commands ocropus-* are
standardized:
 * Put different import statements into different lines
 * Group them into (1) standard imports, (2) third party
   imports, (3) local modules
 * Avoid the "from pylab import *"
 * Use rather "import numpy as np", "import matplotlib.pyplot as plt"

Moreover, some unused import statements were deleted and a very
few nits for readibility are done.

This work is heavily based on the work by @QuLogic in the branch
https://github.com/QuLogic/ocropy/commits/standard-import

All commands from the current pipeline are tested to still work
in the usual calls (especially run-test and run-rtrain).
@zuphilip

This comment has been minimized.

Show comment
Hide comment
@zuphilip

zuphilip Apr 28, 2017

Collaborator

I tried to automatically check the addition of a namespace to the relevant functions (i.e. these are the ones I found so far in the ocropus code) with these lines:

# used functions from numpy need now a prefix 'np.'
perl -ne 'print "$. $_" if /([^.]|^)(amax|amin|arange|array|asarray|clip|dtype\(|isnan|linspace|maximum\(|mean|median\(|minimum\(|ones|random|rand|randn|randint|transpose|var|vstack|zeros)/' ocropus-*

# used functions from matplotlib.pyplot need now a prefix 'plt.'
perl -ne 'print "$. $_" if /([^.]|^)(clf|cm.gray|cm.hot|draw|figure|gca|ginput|gray\(|imshow|\bion\(|\bplot|rc\(|rcParams|savefig|subplot|title)/' ocropus-*
Collaborator

zuphilip commented Apr 28, 2017

I tried to automatically check the addition of a namespace to the relevant functions (i.e. these are the ones I found so far in the ocropus code) with these lines:

# used functions from numpy need now a prefix 'np.'
perl -ne 'print "$. $_" if /([^.]|^)(amax|amin|arange|array|asarray|clip|dtype\(|isnan|linspace|maximum\(|mean|median\(|minimum\(|ones|random|rand|randn|randint|transpose|var|vstack|zeros)/' ocropus-*

# used functions from matplotlib.pyplot need now a prefix 'plt.'
perl -ne 'print "$. $_" if /([^.]|^)(clf|cm.gray|cm.hot|draw|figure|gca|ginput|gray\(|imshow|\bion\(|\bplot|rc\(|rcParams|savefig|subplot|title)/' ocropus-*
@ChillarAnand

This comment has been minimized.

Show comment
Hide comment
@ChillarAnand

ChillarAnand Jul 16, 2017

Contributor

Looks good. Any updates on this? We can also use importmagic to automatically fix imports.

Contributor

ChillarAnand commented Jul 16, 2017

Looks good. Any updates on this? We can also use importmagic to automatically fix imports.

Show outdated Hide outdated ocropus-dewarp
Show outdated Hide outdated ocropus-linegen
@zuphilip

This comment has been minimized.

Show comment
Hide comment
@zuphilip

zuphilip Jul 16, 2017

Collaborator

@ChillarAnand Any checking with importmagic would be appreciated. Moreover, I would like to wait for #234.

Collaborator

zuphilip commented Jul 16, 2017

@ChillarAnand Any checking with importmagic would be appreciated. Moreover, I would like to wait for #234.

@ChillarAnand

This comment has been minimized.

Show comment
Hide comment
@ChillarAnand

ChillarAnand Jul 18, 2017

Contributor

importmagic doesnt work out of the box. I have written a wrapper script and it works only on Python 3.

Any timeline for this PR?

Contributor

ChillarAnand commented Jul 18, 2017

importmagic doesnt work out of the box. I have written a wrapper script and it works only on Python 3.

Any timeline for this PR?

@zuphilip

This comment has been minimized.

Show comment
Hide comment
@zuphilip

zuphilip Jul 18, 2017

Collaborator

Okay, I understand. Actually, we still concentrate on Python 2 and the PR you mentioned is for a separate Python 3 branch. In general it might be hard work to have the whole OCRopus work for Python 2 and Python 3.

Collaborator

zuphilip commented Jul 18, 2017

Okay, I understand. Actually, we still concentrate on Python 2 and the PR you mentioned is for a separate Python 3 branch. In general it might be hard work to have the whole OCRopus work for Python 2 and Python 3.

@kba kba merged commit d823ba4 into tmbdev:master Sep 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zuphilip

This comment has been minimized.

Show comment
Hide comment
@zuphilip

zuphilip Sep 29, 2017

Collaborator

Wow, that was fast :-) I hope that no error was created in the last round of solving merge conflict, because I didn't test it myself much, but simply relied on Travis and the extended tests we have now...

Collaborator

zuphilip commented Sep 29, 2017

Wow, that was fast :-) I hope that no error was created in the last round of solving merge conflict, because I didn't test it myself much, but simply relied on Travis and the extended tests we have now...

@zuphilip zuphilip deleted the zuphilip:imports branch Sep 29, 2017

@kba

This comment has been minimized.

Show comment
Hide comment
@kba

kba Sep 29, 2017

Collaborator

:shipit: :shipit: :shipit:

But seriously, if we'll see regressions from this, it will be fairly obvious, missing imports or undeclared vars.

Collaborator

kba commented Sep 29, 2017

:shipit: :shipit: :shipit:

But seriously, if we'll see regressions from this, it will be fairly obvious, missing imports or undeclared vars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment