search

tulip-control / polytope

Geometric operations on polytopes of any dimension
https://pypi.org/project/polytope
Other
74 stars 19 forks source link

PEP8 Conformity #33

Closed carterbox closed 7 years ago

carterbox commented 7 years ago

I was reading through the code in preparation for adding rotations, and along the way I fixed some PEP8 stuff and removed a redundant logical operator around line 1300 of polytope.py.

Additionally, running the tests using nosetests required that I change permissions from 644 to 755 on the test scripts and I don't think there is any security risk because only the user has write permission.

slivingston commented 7 years ago

regarding changing permissions, can you describe your testing platform or provide terminal output that shows the error that you encountered?

carterbox commented 7 years ago

You're right. There was no problem using nosetests. I can't remember why I changed those permissions then. I should write more detailed commit notes. I've removed that commit from the pull request.

carterbox commented 7 years ago

Also, this build is failing because of a problem installing cvopt on the Travis virtual machine. I'm pretty sure I didn't alter anything that would have affected this because I didn't change the TravisCI build file.

slivingston commented 7 years ago

re https://github.com/tulip-control/polytope/pull/33#issuecomment-270189908, yes, the error is not caused by changes from this pull request. I tried to re-run tests on Travis CI against the tips of master and dev branches; both failed, but had previously succeeded.

slivingston commented 7 years ago

The release of cvxopt v1.1.9 to PyPI occurred on 30 Nov 2016 (as listed at https://pypi.python.org/pypi/cvxopt/1.1.9). The API of cvxopt changed in a way that is not backwards compatible, hence the Travis CI failures.

As a near-future solution, I changed the Travis CI configuration to have pip install cvxopt==1.1.8 on dev branch (commit https://github.com/tulip-control/polytope/commit/38350230519327f471fefe69d2241e0f9ca744e0). I am going to open a pull request for discussion about the moving to 1.1.9

slivingston commented 7 years ago

@carterbox Thanks for your work on improving the code style. I made several minor comments. If you are OK with the additional changes that I propose, then I can merge and apply them, or if you want you can make the changes on your branch (and afterward I merge).

carterbox commented 7 years ago

@slivingston I can make the changes later tonight.

slivingston commented 7 years ago

@carterbox are there any more changes coming? You addressed all of my comments, so this is ready to merge as far as I am concerned.

carterbox commented 7 years ago

@slivingston I'm done now.

johnyf commented 7 years ago

I think that the PR can be rebased on dev to squash the changes.

slivingston commented 7 years ago

@johnyf I was not going to ask for rebasing for the sake of development speed compared with relatively low impact on commit history.

@carterbox if it is easy enough, can you rebase on the tip of dev branch? If you do not know how and want to learn, I can iterate with you off of GitHub, e.g., via email, IRC, or Cryptocat.

slivingston commented 7 years ago

re https://github.com/tulip-control/polytope/pull/33/#pullrequestreview-16936116, @johnyf to which lines are you referring?

slivingston commented 7 years ago

the URL in my previous question did not render well; I am asking about your comment "Whitespace around binary operators...".

johnyf commented 7 years ago

About rebasing, I realized after making that comment that this is about the dev branch, so we can skip rebasing and do a final rebase before the next release.

About the source lines, I refer to, for example, https://github.com/tulip-control/polytope/pull/33/commits/3fb3d150270b0ed3010d695f51f3be5c0bf6bd7b#diff-568a32ef43ad350e3dddd07731579142R1867, https://github.com/tulip-control/polytope/pull/33/commits/3fb3d150270b0ed3010d695f51f3be5c0bf6bd7b#diff-568a32ef43ad350e3dddd07731579142R2166.

The lines from PEP8 are:

"Always surround these binary operators with a single space on either side: assignment ( = ), augmented assignment ( += , -= etc.), comparisons ( == , < , > , != , <> , <= , >= , in , not in , is , is not ), Booleans ( and , or , not ). "

and:

"If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator.

Yes:

i = i + 1
submitted += 1
x = x*2 - 1
hypot2 = x*x + y*y
c = (a+b) * (a-b)

"

I should note that those unbalanced spaces around binary operators are an old style of mine that I carried over from writing Matlab and LaTeX code. After some time of writing Python, I stopped this habit.

carterbox commented 7 years ago

@slivingston I can rebase tonight at 9PST.

slivingston commented 7 years ago

@carterbox no hurry. Notify me when you think that it is ready to merge.

slivingston commented 7 years ago

@johnyf (re https://github.com/tulip-control/polytope/pull/33#issuecomment-273249857) I might have missed something, but the examples that you gave are consistent with there being space around binary operators, so they are not examples of code style that should be changed.

However, after applying changes from this PR, there remain instances where the desired spacing style is not followed. @johnyf, if you are referring to those, I think that we should merge this PR and handle those separately, e.g., while refactoring.

johnyf commented 7 years ago

Indeed, the changes are in accord with PEP8. I'm sorry for the confusion: the reason I made my comment was to support some of the changes based on PEP8, after reading the "because why not?" in the commit message of 3fb3d150270b0ed3010d695f51f3be5c0bf6bd7b.

carterbox commented 7 years ago

@slivingston Rebasing is done. Merge when Travis passes.

slivingston commented 7 years ago

I just noticed that a new commit was added at Mon Jan 16 23:30:54 2017 -0800. I am reviewing those changes now.

slivingston commented 7 years ago

I only had one comment on the new commit, and it must be addressed.

At this stage, I think that clarity of change history would be substantially improved by squashing the three commits that come after the first. Ideally, there would be only two commits:

  1. Remove redundant comparison (as at https://github.com/tulip-control/polytope/pull/33/commits/5d73b690296e80d5a75895d91349216e85a60ca3#diff-568a32ef43ad350e3dddd07731579142L1309)
  2. Adjust space and line continuations to improve code style, following PEP 8.

However, picking out the redundant comparison from your original first commit might require excessive effort. Instead, you can easily:

  1. address my comment about the new commit by creating yet another commit in your fork;
  2. then, rebase, keep the original first commit, and squash all of the others.
  3. during the rebase, consider using commit messages that begin with imperative verbs (e.g., "Improve code style, following PEP 8").

@carterbox I appreciate your patience. The main topic of this PR is code style, so my comments are directly critical about it here.

johnyf commented 7 years ago

I agree with the suggestion to squash, and that was one the main reasons I had in mind when discussing rebasing. In general, I prefer that history be as clean as possible by the time it reaches master. The discussion has touched development practices, so it seems relevant to mention the developer's guide of tulip.

The commit abbreviations I use originate from numpy (as I learned from @slivingston a while ago), and I've refined them a bit. Overall, I use:

API: backward incompatible change

BIB: biliography (for BibTeX files) BIN: for generated files (usually those are binaries) BLD: related to building BUG: error correction

DEP: deprecate something, or remove a deprecated object DEV: development utility DOC: documentation (docstrings too) DRAFT: to be rewritten / fixed up (to be rebased, never in master)

ENH: enhancement EXP: experimental (to be rebased, never in master)

IMG: changes to sources of images (for example, SVG files)

MAI: maintenance

PEP8: style convention PEP: change related to PEP

REF: refactoring REL: release-related REV: revert an earlier commit

STY: style correction

TST: testing

Deciding which prefix from the above to use is not always straightforward, but doing so is a good exercise. I prefer choosing the more severe one applicable (usually API instead of MAI). For example, what distinguishes REF from MAI? REF should clearly be a refactoring that produces code that is (for most practical purposes) equivalent, with the equivalence being clearly evident.