sagemath / sage

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

QQbar decorators should handle sequences #29468

Closed mwageringel closed 4 years ago

mwageringel commented 4 years ago

The forward_map and reverse_map in sage/rings/qqbar_decorators.py do not currently handle polynomial sequences. Because of that, the call to groebner_basis in a polynomial ring over QQbar only returns a list, not a PolynomialSequence, which can be unexpected.

sage: J = QQbar['x,y'].ideal('x^2 - y')
sage: type(J.groebner_basis())
<class 'list'>

The decorator should handle polynomial sequences and should in particular preserve properties such as immutability.

Component: algebra

Keywords: qqbar

Author: Travis Scholl

Branch/Commit: c3e7ed2

Reviewer: Markus Wageringel

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

DaveWitteMorris commented 4 years ago
comment:1

I think it will suffice to modify the definitions of forward_map and reverse_map by adding elif clauses to deal with items that are a PolynomialSequence or a Sequence.

tscholl2 commented 4 years ago

Branch: u/tscholl2/29468

tscholl2 commented 4 years ago

Author: Travis Scholl

tscholl2 commented 4 years ago

Commit: ce09d1d

tscholl2 commented 4 years ago
comment:2

I attempted to handle Sequences and PolynomialSequences, but I'm not exactly sure how to test things like preserving immutability. Let me know if my small test is not enough or if there is some other problem.

mwageringel commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-The `forward_map` and `reverse_map` in `sage/rings/qqbar_decorators.py` do not currently handle polynomial sequences. Because of that, the call to `groebner_basis` in a polynomial ring over `QQbar` only returns a list, not a `Sequence`, which can be unexpected.
+The `forward_map` and `reverse_map` in `sage/rings/qqbar_decorators.py` do not currently handle polynomial sequences. Because of that, the call to `groebner_basis` in a polynomial ring over `QQbar` only returns a list, not a `PolynomialSequence`, which can be unexpected.

sage: J = QQbar['x,y'].ideal('x^2 - y') @@ -6,5 +6,5 @@ <class 'list'>


-The decorator should handle sequences and should in particular preserve properties such as immutability.
+The decorator should handle polynomial sequences and should in particular preserve properties such as immutability.
mwageringel commented 4 years ago

Changed commit from ce09d1d to c3e7ed2

mwageringel commented 4 years ago

Reviewer: Markus Wageringel

mwageringel commented 4 years ago
comment:3

Thank you for working on this. I have made a few more small changes.

The immutability is now checked and preserved via is_immutable(). The constructor for polynomial sequences has additional arguments like cr and cr_str, but these are only syntatic and cannot be checked easily, so I think for now it is not that important to preserve these. In the long run, we might add a .map() method to sequences that retains these options, but we do not need to do this on this ticket.

Moreover, I have removed the handling of Sequence in favour of PolynomialSequence, as a sequence of polynomials over QQbar will always be a PolynomialSequence. I have also added another test case.

Do you agree with the changes?


New commits:

c3e7ed229468: check immutability and add more tests
mwageringel commented 4 years ago

Changed branch from u/tscholl2/29468 to u/gh-mwageringel/29468

tscholl2 commented 4 years ago
comment:4

Replying to @mwageringel:

That sounds good to me! I'm still learning this area of Sage, I will have to look into what cr and cr_str do. I also didn't know that Sequences will always be PolynomialSequences in this context, but I saw the test you added for that.

mwageringel commented 4 years ago
comment:5

Let us get this merged.

vbraun commented 4 years ago

Changed branch from u/gh-mwageringel/29468 to c3e7ed2