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

Bugfix concerning ticket #26421 #29266

Closed
soehms opened this issue Mar 1, 2020 · 10 comments
Closed

Bugfix concerning ticket #26421 #29266

soehms opened this issue Mar 1, 2020 · 10 comments

Comments

@soehms
Copy link
Member

soehms commented Mar 1, 2020

In #26421 factorization of polynomials has been extended to previously untreated cases in which the polynomial's base ring is an integral domain.
The factorization is done over the field of fraction and excepted if it can be coerced into the base ring itself. Since the method base_change of the class Factorzation doesn't check if the unit of the factorization is coerced to a unit again, this implementation is too vastly:

sage: R.<t> = LaurentPolynomialRing(ZZ)
sage: P.<x> = R[]
sage: f = 2*x + 4
sage: f.is_irreducible()
True
sage: F = f.factor(); F
(2) * (x + 2)
sage: F.unit()
2

CC: @tscrim

Component: commutative algebra

Keywords: factorization, integral domain

Author: Sebastian Oehms

Branch/Commit: a21db27

Reviewer: Travis Scrimshaw

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

@soehms soehms added this to the sage-9.1 milestone Mar 1, 2020
@soehms
Copy link
Member Author

soehms commented Mar 1, 2020

@soehms
Copy link
Member Author

soehms commented Mar 1, 2020

Commit: 004c1c4

@soehms
Copy link
Member Author

soehms commented Mar 1, 2020

comment:2

Maybe a better place for the fix would be the method base_change of the class Factorization. But since this leads to the failure of previous doctests (in cases is_unit is not implemented), I preferred the implementation in method factor.


New commits:

004c1c429266 initial version

@soehms
Copy link
Member Author

soehms commented Mar 1, 2020

Author: Sebastian Oehms

@tscrim
Copy link
Collaborator

tscrim commented Mar 2, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Mar 2, 2020

comment:3

One little change: "coerce" to "convert". If it was a coercion map, then the fact it was a unit must be preserved (as coercions must be morphisms). Once you change that, you can set a positive review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2020

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

a21db2729266: correction according to review

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2020

Changed commit from 004c1c4 to a21db27

@soehms
Copy link
Member Author

soehms commented Mar 2, 2020

comment:5

Thanks!

@vbraun
Copy link
Member

vbraun commented Mar 3, 2020

Changed branch from u/soehms/factorization_integral_domain_29266 to a21db27

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

3 participants