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

pycodestyle cleanup in strongly_regular_db.pyx (part 3) #34313

Closed
dcoudert opened this issue Aug 9, 2022 · 10 comments
Closed

pycodestyle cleanup in strongly_regular_db.pyx (part 3) #34313

dcoudert opened this issue Aug 9, 2022 · 10 comments

Comments

@dcoudert
Copy link
Contributor

dcoudert commented Aug 9, 2022

We continue the cleaning of strongly_regular_db.pyx.

This ticket is built on top of #34311 and #34312 to avoid conflicts.

Depends on #34311
Depends on #34312

CC: @fchapoton @tscrim

Component: graph theory

Author: David Coudert

Branch/Commit: f2323c0

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/34313

@dcoudert dcoudert added this to the sage-9.7 milestone Aug 9, 2022
@dcoudert
Copy link
Contributor Author

dcoudert commented Aug 9, 2022

Branch: public/graphs/34313_part3

@dcoudert
Copy link
Contributor Author

dcoudert commented Aug 9, 2022

Commit: 9bf5ba9

@dcoudert
Copy link
Contributor Author

dcoudert commented Aug 9, 2022

New commits:

d629deftrac #34311: clean strongly_regular_db.pyx - part 1
7ef0a02trac #34312: clean strongly_regular_db.pyx - part 2
9bf5ba9trac #34313: clean strongly_regular_db.pyx - part 3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 9, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

f2323c0trac #34313: extra care

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 9, 2022

Changed commit from 9bf5ba9 to f2323c0

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 2, 2022

comment:3

You make many edits like this:

-    if (v == (q**d - 1)*((q**d)//(p**t) + 1)//(q - 1)             and
-        k == q*(q**(d-1) - 1)*((q**(d-1))//(p**t) + 1)//(q - 1)   and
-        l == q*q*(q**(d-2)-1)*((q**(d-2))//(p**t) + 1)//(q - 1) + q - 1):
+    if (v == (q**d - 1)*((q**d)//(p**t) + 1)//(q - 1) and
+            k == q*(q**(d-1) - 1)*((q**(d-1))//(p**t) + 1)//(q - 1) and
+            l == q*q*(q**(d-2)-1)*((q**(d-2))//(p**t) + 1)//(q - 1) + q - 1):
         from sage.graphs.generators.classical_geometries import UnitaryPolarGraph

I am not sure if this improves readability of the code. The change of the positions of and is okay, but the alignment of == in the original code looks nice. The alignment of == is clearly intended by the author, and I think we should respect that.

Let's hear more opinions from others.

@tscrim
Copy link
Collaborator

tscrim commented Sep 2, 2022

comment:4

PEP8 allows for different conventions with how multiple lines of a conditional are done. I am not convinced that the original author intended for the == to be aligned but it is just a byproduct of PEP8 if statement alignment. Since David is the one mostly reading and working with this code, I think we should defer to his choice of style (in particular, as he tries to make this section of the code uniform).

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 2, 2022

comment:5

Okay then.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 2, 2022

Reviewer: Kwankyu Lee

@vbraun
Copy link
Member

vbraun commented Sep 25, 2022

Changed branch from public/graphs/34313_part3 to f2323c0

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

No branches or pull requests

5 participants