Ticket #13903 (closed defect: fixed)

Opened 5 months ago

Last modified 4 months ago

polynomial .reduce returns type int over p-adic field

Reported by: bhutz Owned by: AlexGhitza
Priority: minor Milestone: sage-5.6
Component: algebra Keywords: polynomial reduce
Cc: Work issues:
Report Upstream: N/A Reviewers: Karl-Dieter Crisman
Authors: John Perry Merged in: sage-5.6.beta3
Dependencies: Stopgaps:

Description (last modified by kcrisman) (diff)

The .reduce() function for a polynomial ring can return an 'int' type when the base field is a p-adic field.

R.<y1,y2>=PolynomialRing(Qp(5),2, order='lex')
G=[y1^2 + y2^2, y1*y2 + y2^2, y2^3]
type((y2^3).reduce(G))

It should be returning an element of the polynomial ring.

This was noticed since it causes .variety() to fail.

R.<y1,y2>=PolynomialRing(Qp(5),2, order='lex')
G=[y1^2 + y2^2, y1*y2 + y2^2, y2^3]
I=ideal(G)
I.variety()

Some discussion at:  https://groups.google.com/forum/?fromgroups=#!topic/sage-support/Ar7z2b5cOic

Apply:

Attachments

trac_13903.patch Download (1.2 KB) - added by john_perry 5 months ago.
simple patch + doctest
trac_13903-reviewed.patch Download (1.4 KB) - added by kcrisman 5 months ago.
Apply only this

Change History

comment:1 Changed 5 months ago by john_perry

  • Description modified (diff)

I fixed some formatting issues with the ticket description.

comment:2 Changed 5 months ago by john_perry

  • Description modified (diff)

And promptly broke them, as well. (It looked good before I hit submit, honest!) Okay, I'll try again...

I can work on this, if no one else has their heart set on it.

comment:3 follow-up: ↓ 4 Changed 5 months ago by kcrisman

  • Authors Ben Hutz deleted

Please do! If it's a simple fix of type, maybe I can review it for you.

comment:4 in reply to: ↑ 3 Changed 5 months ago by john_perry

Replying to kcrisman:

Please do! If it's a simple fix of type, maybe I can review it for you.

It would be very easy if I could figure out how I pooched my development environment. I have a working fix, but mercurial doesn't seem to notice the changes. I hate it when this happens.

Changed 5 months ago by john_perry

simple patch + doctest

comment:5 Changed 5 months ago by john_perry

  • Status changed from new to needs_review

I said it was an easy fix. This bug has burned me in other contexts, so it wasn't hard to find and fix.

comment:6 Changed 5 months ago by john_perry

  • Description modified (diff)

comment:7 follow-up: ↓ 9 Changed 5 months ago by kcrisman

I still get

sage: I=ideal(G)
sage: I.variety()
verbose 0 (3482: multi_polynomial_ideal.py, groebner_basis) Warning: falling back to very slow toy implementation.
verbose 0 (1359: multi_polynomial_ideal.py, dimension) Warning: falling back to very slow toy implementation.
verbose 0 (2365: multi_polynomial_ideal.py, variety) Warning: falling back to very slow toy implementation.
verbose 0 (3482: multi_polynomial_ideal.py, groebner_basis) Warning: falling back to very slow toy implementation.
[{y1: O(5^20), y2: O(5^20)}]

but presumably that's okay. I'm uploading a slight refresh of your patch to use our new(ish) :trac: markup, and fixed the other non-occurrence of that in the file (there were several with the new markup already).

Changed 5 months ago by kcrisman

Apply only this

comment:8 Changed 5 months ago by kcrisman

  • Status changed from needs_review to positive_review
  • Reviewers set to Karl-Dieter Crisman
  • Description modified (diff)
  • Authors set to John Perry

Patchbot, apply trac_13903-reviewed.patch

comment:9 in reply to: ↑ 7 Changed 5 months ago by john_perry

Replying to kcrisman:

I still get

sage: I=ideal(G)
sage: I.variety()
verbose 0 (3482: multi_polynomial_ideal.py, groebner_basis) Warning: falling back to very slow toy implementation.
verbose 0 (1359: multi_polynomial_ideal.py, dimension) Warning: falling back to very slow toy implementation.
verbose 0 (2365: multi_polynomial_ideal.py, variety) Warning: falling back to very slow toy implementation.
verbose 0 (3482: multi_polynomial_ideal.py, groebner_basis) Warning: falling back to very slow toy implementation.
[{y1: O(5^20), y2: O(5^20)}]

but presumably that's okay.

If you mean that the warnings are bothering you, then yes, that's okay. Unless I misread the Singular manual, it doesn't deal with Qp, though I could be wrong (I know next to nothing about p-adics, and Singular does deal with Zp). If Singular DOES implement Qp, then we haven't yet implemented that interface. That should be another ticket, though, because this bug would likely pop up even if we weren't in Qp.

I'm uploading a slight refresh of your patch to use our new(ish) :trac: markup...

Hunh. I didn't know about that. I wonder if I can remember it for the future... ;-)

comment:10 Changed 5 months ago by bhutz

Thanks. This looked good on my tests as well.

Yes, that result from .variety() is the correct final answer, well really the result is the point (1:0:0) in projective space Qp, but up to precision that is what is returned. The warnings I ignored ;)

comment:11 Changed 4 months ago by jdemeyer

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-5.6.beta3
Note: See TracTickets for help on using tickets.