#29468 closed enhancement (fixed)

QQbar decorators should handle sequences

Reported by: gh-mwageringel Owned by:
Priority: minor Milestone: sage-9.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:

Status badges

Description (last modified by gh-mwageringel)

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 gh-DaveWitteMorris

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.

comment:2 Changed 19 months ago by tscholl2

  • Authors set to Travis Scholl
  • Branch set to u/tscholl2/29468
  • Commit set to ce09d1dc28e6a372b7ab1b3535e8142faceb10a8
  • Status changed from new to needs_review

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.

comment:3 follow-up: Changed 19 months ago by gh-mwageringel

  • Branch changed from u/tscholl2/29468 to u/gh-mwageringel/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:

c3e7ed229468: check immutability and add more tests

comment:4 in reply to: ↑ 3 Changed 19 months ago by tscholl2

Replying to gh-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.

comment:5 Changed 17 months ago by gh-mwageringel

  • Status changed from needs_review to positive_review

Let us get this merged.

comment:6 Changed 17 months ago by vbraun

  • Branch changed from u/gh-mwageringel/29468 to c3e7ed2293387679c65eb780af5d3591d6f2e4d8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.