Opened 22 months ago
Closed 17 months ago
#29468 closed enhancement (fixed)
QQbar decorators should handle sequences
Reported by:  ghmwageringel  Owned by:  

Priority:  minor  Milestone:  sage9.2 
Component:  algebra  Keywords:  qqbar, beginner 
Cc:  Merged in:  
Authors:  Travis Scholl  Reviewers:  Markus Wageringel 
Report Upstream:  N/A  Work issues:  
Branch:  c3e7ed2 (Commits, GitHub, GitLab)  Commit:  c3e7ed2293387679c65eb780af5d3591d6f2e4d8 
Dependencies:  Stopgaps: 
Description (last modified by )
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.
Change History (6)
comment:1 Changed 20 months ago by
comment:2 Changed 19 months ago by
 Branch set to u/tscholl2/29468
 Commit set to ce09d1dc28e6a372b7ab1b3535e8142faceb10a8
 Status changed from new to needs_review
I attempted to handle Sequence
s and PolynomialSequence
s, 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.
comment:3 followup: ↓ 4 Changed 19 months ago by
 Branch changed from u/tscholl2/29468 to u/ghmwageringel/29468
 Commit changed from ce09d1dc28e6a372b7ab1b3535e8142faceb10a8 to c3e7ed2293387679c65eb780af5d3591d6f2e4d8
 Description modified (diff)
 Reviewers set to Markus Wageringel
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:
c3e7ed2  29468: check immutability and add more tests

comment:4 in reply to: ↑ 3 Changed 19 months ago by
Replying to ghmwageringel:
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 Sequence
s will always be PolynomialSequence
s in this context, but I saw the test you added for that.
comment:5 Changed 17 months ago by
 Status changed from needs_review to positive_review
Let us get this merged.
comment:6 Changed 17 months ago by
 Branch changed from u/ghmwageringel/29468 to c3e7ed2293387679c65eb780af5d3591d6f2e4d8
 Resolution set to fixed
 Status changed from positive_review to closed
I think it will suffice to modify the definitions of
forward_map
andreverse_map
by addingelif
clauses to deal with items that are aPolynomialSequence
or aSequence
.