Opened 5 years ago
Closed 5 years ago
#23955 closed defect (fixed)
trains() forgets the trains of length one.
Reported by:  sbrandhorst  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  quadratic forms  Keywords:  sd91 
Cc:  Merged in:  
Authors:  Simon Brandhorst  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  d7fa2c4 (Commits, GitHub, GitLab)  Commit:  d7fa2c47e7e39210e59944c71e10f949bc329999 
Dependencies:  Stopgaps: 
Description
In sage.modules.genera.genus.canonical_2_adic_trains forgets the trains of length one. Maybe this causes further bugs.
Change History (9)
comment:1 Changed 5 years ago by
 Component changed from PLEASE CHANGE to quadratic forms
 Keywords sd91 added
comment:2 Changed 5 years ago by
 Branch set to u/sbrandhorst/trains___forgets_the_trains_of_length_one_
comment:3 Changed 5 years ago by
 Commit set to 711bbf05fd5d2d805d609b44f3b1a559f1e0c946
 Status changed from new to needs_review
comment:4 Changed 5 years ago by
 Branch changed from u/sbrandhorst/trains___forgets_the_trains_of_length_one_ to u/roed/trains___forgets_the_trains_of_length_one_
comment:5 Changed 5 years ago by
 Commit changed from 711bbf05fd5d2d805d609b44f3b1a559f1e0c946 to 8e8c9cca6f92fd5fcc268c3d9b456b4039c6a1fa
 Reviewers set to David Roe
Most of my changes were superficial, but I did find what I thought was a typo. You used to have
if left_neighbor == 1 or symbol[i] == 1 or right_neighbor==1:
which I changed to
if left_neighbor == 1 or symbol[i][3] == 1 or right_neighbor==1:
based on context. If this is correct, you're happy with the rest of my changes, and tests pass, positive review.
New commits:
43cfcc1  Merge branch 'u/sbrandhorst/trains___forgets_the_trains_of_length_one_' of git://trac.sagemath.org/sage into t/23955/trains

8e8c9cc  Small fixes to new trains code

comment:6 Changed 5 years ago by
 Status changed from needs_review to positive_review
Thank you for the changes. It is a lot cleaner now. Tests pass on my machine and the patchbot only complains about long tests in other unrelated components. Seems good.
comment:7 Changed 5 years ago by
 Commit changed from 8e8c9cca6f92fd5fcc268c3d9b456b4039c6a1fa to d7fa2c47e7e39210e59944c71e10f949bc329999
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
d7fa2c4  Fix tab character in index.rst

comment:8 Changed 5 years ago by
 Status changed from needs_review to positive_review
No, that was a real problem in the tests with a TAB character. I've fixed it now.
comment:9 Changed 5 years ago by
 Branch changed from u/roed/trains___forgets_the_trains_of_length_one_ to d7fa2c47e7e39210e59944c71e10f949bc329999
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Rewrote canonical_2_adic_trains()
Removed unnecessary computations of compartments for the train function.