Opened 2 years ago

Closed 2 years ago

#23955 closed defect (fixed)

trains() forgets the trains of length one.

Reported by: sbrandhorst Owned by:
Priority: major Milestone: sage-8.1
Component: quadratic forms Keywords: sd91
Cc: Merged in:
Authors: Simon Brandhorst Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: d7fa2c4 (Commits) 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 2 years ago by sbrandhorst

  • Component changed from PLEASE CHANGE to quadratic forms
  • Keywords sd91 added

comment:2 Changed 2 years ago by sbrandhorst

  • Branch set to u/sbrandhorst/trains___forgets_the_trains_of_length_one_

comment:3 Changed 2 years ago by sbrandhorst

  • Commit set to 711bbf05fd5d2d805d609b44f3b1a559f1e0c946
  • Status changed from new to needs_review

New commits:

22c83b7Rewrote canonical_2_adic_trains()
711bbf0Removed unnecessary computations of compartments for the train function.

comment:4 Changed 2 years ago by roed

  • 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 2 years ago by roed

  • 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:

43cfcc1Merge branch 'u/sbrandhorst/trains___forgets_the_trains_of_length_one_' of git://trac.sagemath.org/sage into t/23955/trains
8e8c9ccSmall fixes to new trains code

comment:6 Changed 2 years ago by sbrandhorst

  • 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 2 years ago by git

  • 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:

d7fa2c4Fix tab character in index.rst

comment:8 Changed 2 years ago by roed

  • 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 2 years ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.