Opened 7 years ago

Closed 6 years ago

#13438 closed task (fixed)

univariate polynomial _xgcd only over fields

Reported by: saraedum Owned by: AlexGhitza
Priority: trivial Milestone: sage-5.12
Component: basic arithmetic Keywords: gcd, xgcd, beginner sd51
Cc: Merged in: sage-5.12.beta2
Authors: Julian Rueth Reviewers: Travis Scrimshaw, Michiel Kosters
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by tscrim)

sage.rings.polynomial.polynomial_element.Polynomial provides an implementation for _xgcd. This implementation is not correct for polynomials over arbitrary rings. Therefore it should be moved to sage.rings.polynomial.polynomial_element_generic.Polynomial_generic_field.

The way it currently is, doesn't cause any bugs (except for one which already has a stopgap warning) because only elements of a PID call the _xgcd method.


Apply: trac_13438_header.patch

Attachments (3)

trac_13438.patch (5.0 KB) - added by saraedum 7 years ago.
trac_13438_beta.patch (4.6 KB) - added by mkosters 6 years ago.
trac_13438_header.patch (4.8 KB) - added by mkosters 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by saraedum

  • Keywords removed

comment:2 Changed 7 years ago by saraedum

  • Description modified (diff)
  • Keywords gcd beginner added

comment:3 Changed 7 years ago by saraedum

  • Authors set to Julian Rueth

comment:4 Changed 7 years ago by saraedum

  • Dependencies set to #13439

comment:5 Changed 7 years ago by saraedum

  • Dependencies #13439 deleted

Changed 7 years ago by saraedum

comment:6 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:7 Changed 7 years ago by saraedum

  • Status changed from new to needs_review

comment:8 Changed 7 years ago by tscrim

  • Status changed from needs_review to needs_work

Two minor things I'd like to see changed:

  • The INPUT: bullet is indented one to many times
  • Change the OUTPUT: to
    Polynomials ``g``, ``u``, and ``v`` such that ``g = u * self + v * other``. 
    

Thanks.

Changed 6 years ago by mkosters

comment:9 Changed 6 years ago by mkosters

  • Status changed from needs_work to needs_review

Beta patch should replace original patch. Original patch fails due to intermediate changes in sage (as of 5.10). Beta patch also includes tscrim's suggested doc changes.

comment:10 Changed 6 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

The patch is missing the header information. You'll need to run

sage -hg qrefresh -e
sage -hg export trac_13438_beta.patch > path/to/sage/devel/sage/.hg/patches/trac_13438_beta.patch

(or save it to wherever you'd like to upload the patch from). You should also add yourself as a reviewer of the patch with your real name.

Thanks,
Travis

Changed 6 years ago by mkosters

comment:11 Changed 6 years ago by mkosters

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Michiel Kosters

The new patch (trac_13438_header.patch​) includes header and commit.

comment:12 Changed 6 years ago by tscrim

  • Description modified (diff)

Looks good to me. Thank you.

For patchbot:

Apply: trac_13438_header.patch​

comment:13 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:14 Changed 6 years ago by mstreng

  • Keywords sd51 added

comment:15 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:16 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.