sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 462 forks source link

make PolynomialQuotientRing_generic inherit from QuotientRing_generic #34463

Closed yyyyx4 closed 1 year ago

yyyyx4 commented 2 years ago

This will make it easier to write code that works uniformly for all kinds of quotient rings.

(Example: Polynomial quotient rings currently do not have the .cover() and .defining_ideal() methods, which are very useful when writing generic quotient-ring code.)

Component: algebra

Author: Lorenz Panny

Branch/Commit: b56ebf9

Reviewer: Marc Mezzarobba

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

yyyyx4 commented 2 years ago

Commit: 0d42205

yyyyx4 commented 2 years ago

Author: Lorenz Panny

yyyyx4 commented 2 years ago

Branch: public/make_PolynomialQuotientRing_inherit_from_QuotientRing

roed314 commented 2 years ago
comment:2

There seems to be a test failure

File "sage/schemes/elliptic_curves/hom_velusqrt.py", line 829, in sage.schemes.elliptic_curves.hom_velusqrt.EllipticCurveHom_velusqrt
Failed example:
    any(map(check, psi.codomain().isomorphisms(phi.codomain())))
Expected:
    True
Got:
    False

and some linting failures

sage/schemes/elliptic_curves/hom_velusqrt.py:325:9: E306 expected 1 blank line before a nested definition, found 0
sage/schemes/elliptic_curves/hom_velusqrt.py:327:9: E306 expected 1 blank line before a nested definition, found 0
sage/functions/special.py:867:12: W605 invalid escape sequence '\p'
sage/functions/special.py:867:19: W605 invalid escape sequence '\p'
sage/algebras/clifford_algebra.py:2987:21: E721 do not compare types, use 'isinstance()'

The linting problems don't look like they're due to this ticket, but I'm not sure about the test failure.

yyyyx4 commented 2 years ago
comment:3

Thanks for having a look. The test failure is #34467, and the linter errors should go away with #34466.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

b56ebf9Merge tag '9.7' into public/make_PolynomialQuotientRing_inherit_from_QuotientRing
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 0d42205 to b56ebf9

mezzarobba commented 2 years ago

Reviewer: Marc Mezzarobba

mezzarobba commented 2 years ago
comment:5

I think is_Class functions that just perform a type test are considered obsolete, so I would write if not isinstance(x, PolynomialQuotientRing_generic) instead of calling is_PolynomialQuotientRing. Lgtm otherwise!

vbraun commented 1 year ago

Changed branch from public/make_PolynomialQuotientRing_inherit_from_QuotientRing to b56ebf9