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

make NumberFieldFractionalIdeal inherit from Ideal_fractional #32380

Closed
yyyyx4 opened this issue Aug 15, 2021 · 17 comments
Closed

make NumberFieldFractionalIdeal inherit from Ideal_fractional #32380

yyyyx4 opened this issue Aug 15, 2021 · 17 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 15, 2021

We have an Ideal_fractional parent class, but the most important type of fractional ideals in Sage (namely, for number fields) does not inherit from it. Let's change this, so that we can reliably distinguish fractional ideals from ideals.

(I haven't found any other fractional ideals in Sage that are not already children of Ideal_fractional.)

This patch should result in absolutely no change in behaviour, except that isinstance(I, Ideal_fractional) now returns True when I is a NumberFieldFractionalIdeal.

Component: number fields

Keywords: fractional ideals

Author: Lorenz Panny

Branch/Commit: 55cfe14

Reviewer: Frédéric Chapoton

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

@yyyyx4 yyyyx4 added this to the sage-9.5 milestone Aug 15, 2021
@fchapoton
Copy link
Contributor

comment:2

needs a doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2021

Changed commit from aa898ad to 8039ce4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2021

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

8039ce4add doctest for #32380

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 15, 2021

comment:4

Done.

@fchapoton
Copy link
Contributor

comment:5

ok

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@vbraun
Copy link
Member

vbraun commented Oct 14, 2021

comment:6

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2021

Changed commit from 8039ce4 to 65805c5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2021

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

65805c5Merge tag '9.5.beta3' into public/make_NumberFieldFractionalIdeal_inherit_from_Ideal_fractional

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 15, 2021

comment:8

Thanks, fixed.

@tscrim
Copy link
Collaborator

tscrim commented Nov 3, 2021

comment:9

One (seems trivial) failing doctest according to the patchbot:

sage -t --long --random-seed=43805222569518434108329399959452339425 src/sage/rings/polynomial/polynomial_element_generic.py
**********************************************************************
File "src/sage/rings/polynomial/polynomial_element_generic.py", line 783, in sage.rings.polynomial.polynomial_element_generic.Polynomial_generic_sparse.quo_rem
Failed example:
    f.quo_rem(g)
Expected:
    Traceback (most recent call last):
    ...
    ArithmeticError: Division non exact (consider coercing to polynomials over the fraction field)
Got:
    (-y^5 + 2*y^2, y^3 - 2*x^2*y^2 - y)
**********************************************************************

So some functionality is apparently changed for something that did not work before but now gives a result. Seems like a good thing.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 3, 2021

comment:10

Some quick testing suggests the failure is random (unrelated to this ticket, in any case). I've opened #32816 for this.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2021

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

55cfe14Merge tag '9.5.beta5' into public/make_NumberFieldFractionalIdeal_inherit_from_Ideal_fractional

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2021

Changed commit from 65805c5 to 55cfe14

@tscrim
Copy link
Collaborator

tscrim commented Nov 3, 2021

comment:12

Thanks. Then I am setting this back to a positive review.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 3, 2021

comment:13

Thank you!

@vbraun
Copy link
Member

vbraun commented Nov 15, 2021

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

4 participants