#27726 closed enhancement (fixed)

some cleanup in quadratic forms

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-8.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 chapoton

  • Branch set to u/chapoton/27726
  • Commit set to 63f4059d51a6b5997df69961db00d17d9e58d828
  • Status changed from new to needs_review

New commits:

63f4059some cleanup in quadratic forms

comment:2 Changed 13 months ago by chapoton

  • Cc tscrim added

green bot, please review

comment:3 Changed 13 months ago by tscrim

  • 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 p-neighbors, and computing classes in a genus. ##
-####################################################################################
+# ######################################################################
+# Routines used for understanding p-neighbors 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 cc-ing 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 sbrandhorst

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 git

  • Commit changed from 63f4059d51a6b5997df69961db00d17d9e58d828 to c0fc402903a411247d25056d5f67688aedf8f36d

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

c0fc402trac 27726 some details

comment:6 Changed 13 months ago by chapoton

some fixes as suggested

comment:7 Changed 13 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:8 Changed 13 months ago by vbraun

  • Branch changed from u/chapoton/27726 to c0fc402903a411247d25056d5f67688aedf8f36d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.