Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug in LaurentPolynomial_univariate.quo_rem #34330

Closed
mantepse opened this issue Aug 10, 2022 · 17 comments
Closed

bug in LaurentPolynomial_univariate.quo_rem #34330

mantepse opened this issue Aug 10, 2022 · 17 comments

Comments

@mantepse
Copy link
Collaborator

We currently doctest

def quo_rem(self, right_r):
...
            sage: (t^-2 + 3 + t).quo_rem(t^-4 + t)
            (0, 1 + 3*t^2 + t^3)

I think we want

sage: q, r = num.quo_rem(den)
sage: num == den * q + r
True

Component: algebra

Author: Martin Rubey

Branch/Commit: 8798139

Reviewer: Dave Morris, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/34330

@mantepse mantepse added this to the sage-9.7 milestone Aug 10, 2022
@mantepse
Copy link
Collaborator Author

@mantepse
Copy link
Collaborator Author

New commits:

80e3478fix offset of remainder

@mantepse
Copy link
Collaborator Author

Commit: 80e3478

@fchapoton
Copy link
Contributor

comment:3

missing space here ``q``and

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

Changed commit from 80e3478 to e4479ed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

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

e4479edfix missing space

@trevorkarn
Copy link
Contributor

comment:5

Should other be right_r? Other than that it looks good to me.

@DaveWitteMorris
Copy link
Member

comment:6

Please make an additional minor corrections to the docstring (in addition to comment:5):

  • "Attempts to divide self by right and returns" should be "Attempt to divide self by right_r and return" or, more simply (and I think better), "Divide self by right_r and return".

Related ticket: #31257, which fixed a similar bug in the multivariate case. We briefly discussed the single-variable case there and I thought it was ok, but I was being stupid.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

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

3da6b3dbetter docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

Changed commit from e4479ed to 3da6b3d

@tscrim
Copy link
Collaborator

tscrim commented Aug 11, 2022

@tscrim
Copy link
Collaborator

tscrim commented Aug 11, 2022

Reviewer: David Morris, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 11, 2022

Changed commit from 3da6b3d to 8798139

@tscrim
Copy link
Collaborator

tscrim commented Aug 11, 2022

comment:8

I made a few other small tweaks to the doctests to make it easier for me to read and added a slightly more non-trivial test.


New commits:

8798139Adding a slightly more complicated doctest and other small tweaks.

@DaveWitteMorris
Copy link
Member

comment:9

LGTM.

@DaveWitteMorris
Copy link
Member

Changed reviewer from David Morris, Travis Scrimshaw to Dave Morris, Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

Changed branch from public/rings/laurent_quo_rem_bug-34330 to 8798139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants