Opened 13 months ago
Closed 13 months ago
#27726 closed enhancement (fixed)
some cleanup in quadratic forms
Reported by:  chapoton  Owned by:  

Priority:  minor  Milestone:  sage8.8 
Component:  quadratic forms  Keywords:  
Cc:  tscrim, sbrandhorst  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  c0fc402 (Commits)  Commit:  c0fc402903a411247d25056d5f67688aedf8f36d 
Dependencies:  Stopgaps: 
Description
some pyflakes, pep, and code cleanup in just a few files there
Change History (8)
comment:1 Changed 13 months ago by
 Branch set to u/chapoton/27726
 Commit set to 63f4059d51a6b5997df69961db00d17d9e58d828
 Status changed from new to needs_review
comment:3 Changed 13 months ago by
 Cc sbrandhorst added
 Reviewers set to Travis Scrimshaw
I am not quite in favor of these changes:
#from sage.quadratic_forms.quadratic_form import QuadraticForm ## This creates a circular import! =( #################################################################################### ## Routines used for understanding pneighbors, and computing classes in a genus. ## #################################################################################### +# ###################################################################### +# Routines used for understanding pneighbors and computing classes in a genus. +# ######################################################################
The first one deserves to be there as a warning for future code. For the latter, the first looks better visually IMO. However, that one is clearly bikeshedding, so I won't push it.
 ## Find a (dual) vector w with B(v,w) != 0 (mod p)  v_dual = B2 * vector(v) ## We want the dot product with this to not be divisible by 2*p. + # Find a (dual) vector w with B(v,w) != 0 (mod p) + v_dual = B2 * vector(v) + # We want the dot product with this to not be divisible by 2*p.
For this change, I feel the comment is better served before the v_dual
line if you do not want it on the same line.
if hasattr(self, "__split_local_cover"): if is_QuadraticForm(self.__split_local_cover): ## Here the computation has been done. return self.__split_local_cover  elif isinstance(self.__split_local_cover, Integer): ## Here it indexes the values already tried! + elif self.__split_local_cover in ZZ: ## Here it indexes the values already tried! current_length = self.__split_local_cover + 1 Length_Max = current_length + 5 else:
I am slightly worried about this change as ZZ
and QQ
elements are slightly different and this can matter. I don't think so, but I am ccing Simon to see if he agrees that this change is completely safe (and to look over the other changes in case he has some other comments).
comment:4 Changed 13 months ago by
grepping __split_local_cover
shows that is is either a quadratic form
or an Integer. The change mentioned should be safe.
Thanks for the cleanup. Note that there is still a lot of broken/incomplete code around in the quadratic forms folder.
comment:5 Changed 13 months ago by
 Commit changed from 63f4059d51a6b5997df69961db00d17d9e58d828 to c0fc402903a411247d25056d5f67688aedf8f36d
Branch pushed to git repo; I updated commit sha1. New commits:
c0fc402  trac 27726 some details

comment:6 Changed 13 months ago by
some fixes as suggested
comment:7 Changed 13 months ago by
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:8 Changed 13 months ago by
 Branch changed from u/chapoton/27726 to c0fc402903a411247d25056d5f67688aedf8f36d
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
some cleanup in quadratic forms